[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

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 {
+	if gtserror.StatusCode(err) >= 400 {
-		// No matter what, log the error
-		// so instance admins have an idea
-		// why something isn't working.
-		log.Info(ctx, err)
-
 		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,12 +1065,18 @@ 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)
+
+			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.
 		if !status.PinnedAt.IsZero() {

internal/federation/dereferencing/status.go

@@ -41,7 +41,7 @@ import (
 // statusUpToDate returns whether the given status model is both updateable
 // (i.e. remote status) and whether it needs an update based on `fetched_at`.
 func statusUpToDate(status *gtsmodel.Status, force bool) bool {
-	if *status.Local {
+	if status.Local != nil && *status.Local {
 		// Can't update local statuses.
 		return true
 	}
@@ -69,16 +69,24 @@ func statusUpToDate(status *gtsmodel.Status, force bool) bool {
 // is beyond a certain interval, the status will be dereferenced. In the case of dereferencing, some low-priority status information may be enqueued for asynchronous fetching,
 // e.g. dereferencing the status thread. Param 'syncParent' = true indicates to fetch status ancestors synchronously. An ActivityPub object indicates the status was dereferenced.
 func (d *Dereferencer) GetStatusByURI(ctx context.Context, requestUser string, uri *url.URL) (*gtsmodel.Status, ap.Statusable, error) {
-	// Fetch and dereference status if necessary.
+
+	// Fetch and dereference / update status if necessary.
 	status, statusable, isNew, err := d.getStatusByURI(ctx,
 		requestUser,
 		uri,
 	)
+
 	if err != nil {
+		if status == nil {
+			// err with no existing
+			// status for fallback.
 			return nil, nil, err
 		}
 
-	if statusable != nil {
+		log.Errorf(ctx, "error updating status %s: %v", uri, err)
+
+	} else if statusable != nil {
+
 		// Deref parents + children.
 		d.dereferenceThread(ctx,
 			requestUser,
@@ -92,7 +100,7 @@ func (d *Dereferencer) GetStatusByURI(ctx context.Context, requestUser string, u
 	return status, statusable, nil
 }
 
-// getStatusByURI is a package internal form of .GetStatusByURI() that doesn't bother dereferencing the whole thread on update.
+// getStatusByURI is a package internal form of .GetStatusByURI() that doesn't dereference thread on update, and may return an existing status with error on failed re-fetch.
 func (d *Dereferencer) getStatusByURI(ctx context.Context, requestUser string, uri *url.URL) (*gtsmodel.Status, ap.Statusable, bool, error) {
 	var (
 		status *gtsmodel.Status
@@ -100,8 +108,9 @@ func (d *Dereferencer) getStatusByURI(ctx context.Context, requestUser string, u
 		err    error
 	)
 
-	// Search the database for existing status with URI.
+	// Search the database for existing by URI.
 	status, err = d.state.DB.GetStatusByURI(
+
 		// request a barebones object, it may be in the
 		// db but with related models not yet dereferenced.
 		gtscontext.SetBarebones(ctx),
@@ -112,7 +121,7 @@ func (d *Dereferencer) getStatusByURI(ctx context.Context, requestUser string, u
 	}
 
 	if status == nil {
-		// Else, search the database for existing by URL.
+		// Else, search database for existing by URL.
 		status, err = d.state.DB.GetStatusByURL(
 			gtscontext.SetBarebones(ctx),
 			uriStr,
@@ -123,14 +132,16 @@ func (d *Dereferencer) getStatusByURI(ctx context.Context, requestUser string, u
 	}
 
 	if status == nil {
-		// Ensure that this isn't a search for a local status.
+		// Ensure not a failed search for a local
-		if uri.Host == config.GetHost() || uri.Host == config.GetAccountDomain() {
+		// status, if so we know it doesn't exist.
-			return nil, nil, false, gtserror.SetUnretrievable(err) // this will be db.ErrNoEntries
+		if uri.Host == config.GetHost() ||
+			uri.Host == config.GetAccountDomain() {
+			return nil, nil, false, gtserror.SetUnretrievable(err)
 		}
 
 		// Create and pass-through a new bare-bones model for deref.
 		return d.enrichStatusSafely(ctx, requestUser, uri, &gtsmodel.Status{
-			Local: func() *bool { var false bool; return &false }(),
+			Local: util.Ptr(false),
 			URI:   uriStr,
 		}, nil)
 	}
@@ -145,21 +156,22 @@ func (d *Dereferencer) getStatusByURI(ctx context.Context, requestUser string, u
 		return status, nil, false, nil
 	}
 
-	// Try to update + deref existing status model.
+	// Try to deref and update existing status model.
 	latest, statusable, isNew, err := d.enrichStatusSafely(ctx,
 		requestUser,
 		uri,
 		status,
 		nil,
 	)
-	if err != nil {
-		log.Errorf(ctx, "error enriching remote status: %v", err)
 
-		// Fallback to existing status.
+	if err != nil {
-		return status, nil, false, nil
+		// fallback to the
+		// existing status.
+		latest = status
+		statusable = nil
 	}
 
-	return latest, statusable, isNew, nil
+	return latest, statusable, isNew, err
 }
 
 // RefreshStatus is functionally equivalent to GetStatusByURI(), except that it requires a pre
@@ -294,12 +306,7 @@ func (d *Dereferencer) enrichStatusSafely(
 		apubStatus,
 	)
 
-	if code := gtserror.StatusCode(err); code >= 400 {
+	if gtserror.StatusCode(err) >= 400 {
-		// No matter what, log the error
-		// so instance admins have an idea
-		// why something isn't working.
-		log.Info(ctx, err)
-
 		if isNew {
 			// This was a new status enrich
 			// attempt which failed before we
@@ -316,7 +323,7 @@ func (d *Dereferencer) enrichStatusSafely(
 		// return the model we had stored already.
 		status.FetchedAt = time.Now()
 		if err := d.state.DB.UpdateStatus(ctx, status, "fetched_at"); err != nil {
-			log.Errorf(ctx, "error updating status fetched_at: %v", err)
+			log.Error(ctx, "error updating %s fetched_at: %v", uriStr, err)
 		}
 	}
 
@@ -408,19 +415,21 @@ func (d *Dereferencer) enrichStatus(
 		return nil, nil, gtserror.Newf("error converting statusable to gts model for status %s: %w", uri, err)
 	}
 
-	// Check if we've previously
+	// Based on the original provided
-	// stored this status in the DB.
+	// status model, determine whether
-	// If we have, it'll be ID'd.
+	// this is a new insert / update.
-	var isNew = (status.ID == "")
+	var isNew bool
-	if isNew {
+
-		// No ID, we haven't stored this status before.
+	if isNew = (status.ID == ""); isNew {
-		// Generate new status ID from the status publication time.
+
+		// Generate new status ID from the provided creation date.
 		latestStatus.ID, err = id.NewULIDFromTime(latestStatus.CreatedAt)
 		if err != nil {
 			log.Errorf(ctx, "invalid created at date (falling back to 'now'): %v", err)
 			latestStatus.ID = id.NewULID() // just use "now"
 		}
 	} else {
+
 		// Reuse existing status ID.
 		latestStatus.ID = status.ID
 	}

internal/federation/dereferencing/thread.go

@@ -19,7 +19,6 @@ package dereferencing
 
 import (
 	"context"
-	"errors"
 	"net/http"
 	"net/url"
 
@@ -27,8 +26,6 @@ import (
 	"github.com/superseriousbusiness/activity/pub"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/config"
-	"github.com/superseriousbusiness/gotosocial/internal/db"
-	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
 	"github.com/superseriousbusiness/gotosocial/internal/gtserror"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
@@ -101,144 +98,85 @@ func (d *Dereferencer) DereferenceStatusAncestors(ctx context.Context, username
 			return nil
 		}
 
-		// Add new log fields for this iteration.
+		l = l.WithField("parent", current.InReplyToURI)
-		l = l.WithFields(kv.Fields{
+		l.Trace("following status ancestor")
-			{"current", current.URI},
-			{"parent", current.InReplyToURI},
-		}...)
-		l.Trace("following status ancestors")
 
-		// Check whether this parent has already been deref'd.
+		// Parse status parent URI for later use.
-		if _, ok := derefdStatuses[current.InReplyToURI]; ok {
+		uri, err := url.Parse(current.InReplyToURI)
-			l.Warn("self referencing status ancestors")
+		if err != nil {
+			l.Warnf("invalid uri: %v", err)
 			return nil
 		}
 
-		// Add this status URI to map of deref'd.
+		// Check whether this parent has already been deref'd.
-		derefdStatuses[current.URI] = struct{}{}
+		if _, ok := derefdStatuses[current.InReplyToURI]; ok {
-
+			l.Warn("self referencing status ancestor")
-		if current.InReplyToID != "" {
+			return nil
-			// We already have an InReplyToID set. This means
-			// the status's parent has, at some point, been
-			// inserted into the database, either because it
-			// is a status from our instance, or a status from
-			// remote that we've dereferenced before, or found
-			// out about in some other way.
-			//
-			// Working on this assumption, check if the parent
-			// status exists, either as a copy pinned on the
-			// current status, or in the database.
-
-			if current.InReplyTo != nil {
-				// We have the parent already, and the child
-				// doesn't need to be updated; keep iterating
-				// from this parent upwards.
-				current = current.InReplyTo
-				continue
 		}
 
-			// Parent isn't pinned to this status (yet), see
+		// Add this status's parent URI to map of deref'd.
-			// if we can get it from the db (we should be
+		derefdStatuses[current.InReplyToURI] = struct{}{}
-			// able to, since it has an ID already).
-			parent, err := d.state.DB.GetStatusByID(
-				gtscontext.SetBarebones(ctx),
-				current.InReplyToID,
-			)
-			if err != nil && !errors.Is(err, db.ErrNoEntries) {
-				// Real db error, stop.
-				return gtserror.Newf("db error getting status %s: %w", current.InReplyToID, err)
-			}
 
-			if parent != nil {
+		// Fetch parent status by current's reply URI, this handles
-				// We got the parent from the db, and the child
+		// case of existing (updating if necessary) or a new status.
-				// doesn't need to be updated; keep iterating
+		parent, _, _, err := d.getStatusByURI(ctx, username, uri)
-				// from this parent upwards.
-				current.InReplyTo = parent
-				current = parent
-				continue
-			}
 
-			// If we arrive here, we know this child *did* have
+		// Check for a returned HTTP code via error.
-			// a parent at some point, but it no longer exists in
+		switch code := gtserror.StatusCode(err); {
-			// the database, presumably because it's been deleted
-			// by another action.
-			//
-			// TODO: clean this up in a nightly task.
-			l.Warn("orphaned status (parent no longer exists)")
-			return nil // Cannot iterate further.
-		}
 
-		// If we reach this point, we know the status has
+		// Status codes 404 and 410 incicate the status does not exist anymore.
-		// an InReplyToURI set, but it doesn't yet have an
+		// Gone (410) is the preferred for deletion, but we accept NotFound too.
-		// InReplyToID, which means that the parent status
+		case code == http.StatusNotFound || code == http.StatusGone:
-		// has not yet been dereferenced.
+			l.Trace("status orphaned")
-		inReplyToURI, err := url.Parse(current.InReplyToURI)
+			current.InReplyToID = ""
-		if err != nil || inReplyToURI == nil {
+			current.InReplyToURI = ""
-			// Parent URI is not something we can handle.
+			current.InReplyToAccountID = ""
-			l.Warn("orphaned status (invalid InReplyToURI)")
+			current.InReplyTo = nil
-			return nil //nolint:nilerr
+			current.InReplyToAccount = nil
-		}
+			if err := d.state.DB.UpdateStatus(ctx,
-
+				current,
-		// Parent URI is valid, try to get it.
-		// getStatusByURI guards against the following conditions:
-		//   - refetching recently fetched statuses (recursion!)
-		//   - remote domain is blocked (will return unretrievable)
-		//   - any http type error for a new status returns unretrievable
-		parent, _, _, err := d.getStatusByURI(ctx, username, inReplyToURI)
-		if err == nil {
-			// We successfully fetched the parent.
-			// Update current status with new info.
-			current.InReplyToID = parent.ID
-			current.InReplyToAccountID = parent.AccountID
-			if err := d.state.DB.UpdateStatus(
-				ctx, current,
 				"in_reply_to_id",
+				"in_reply_to_uri",
 				"in_reply_to_account_id",
 			); err != nil {
 				return gtserror.Newf("db error updating status %s: %w", current.ID, err)
 			}
+			return nil
 
-			// Mark parent as next status to
+		// An error was returned for a status during
-			// work on, and keep iterating.
+		// an attempted NEW dereference, return here.
-			current = parent
+		case err != nil && current.InReplyToID == "":
-			continue
+			return gtserror.Newf("error dereferencing new %s: %w", current.InReplyToURI, err)
-		}
 
-		// We could not fetch the parent, check if we can do anything
+		// An error was returned for an existing parent,
-		// useful with the error. For example, HTTP status code returned
+		// we simply treat this as a temporary situation.
-		// from remote may indicate that the parent has been deleted.
+		// (we fallback to using existing parent status).
-		switch code := gtserror.StatusCode(err); {
+		case err != nil:
-		case code == http.StatusGone:
+			l.Errorf("error getting parent: %v", err)
-			// 410 means the status has definitely been deleted.
-			// Update this status to reflect that, then bail.
-			l.Debug("orphaned status: parent returned 410 Gone")
 
-			current.InReplyToURI = ""
+		// The ID has changed for currently stored parent ID
-			if err := d.state.DB.UpdateStatus(
+		// (which may be empty, if new!) and fetched version.
-				ctx, current,
+		//
+		// Update the current's inReplyTo fields to parent.
+		case current.InReplyToID != parent.ID:
+			l.Tracef("parent changed %s => %s", current.InReplyToID, parent.ID)
+			current.InReplyToAccountID = parent.AccountID
+			current.InReplyToAccount = parent.Account
+			current.InReplyToURI = parent.URI
+			current.InReplyToID = parent.ID
+			if err := d.state.DB.UpdateStatus(ctx,
+				current,
+				"in_reply_to_id",
 				"in_reply_to_uri",
+				"in_reply_to_account_id",
 			); err != nil {
 				return gtserror.Newf("db error updating status %s: %w", current.ID, err)
 			}
-
-			return nil
-
-		case code != 0:
-			// We had a code, but not one indicating deletion, log the code
-			// but don't return error or update the status; we can try again later.
-			l.Warnf("orphaned status: http error dereferencing parent: %v)", err)
-			return nil
-
-		case gtserror.IsUnretrievable(err):
-			// Not retrievable for some other reason, so just
-			// bail for now; we can try again later if necessary.
-			l.Warnf("orphaned status: parent unretrievable: %v)", err)
-			return nil
-
-		default:
-			// Some other error that stops us in our tracks.
-			return gtserror.Newf("error dereferencing parent %s: %w", current.InReplyToURI, err)
 		}
+
+		// Set next parent to use.
+		current.InReplyTo = parent
+		current = current.InReplyTo
 	}
 
 	return gtserror.Newf("reached %d ancestor iterations for %q", maxIter, status.URI)
@@ -336,7 +274,7 @@ stackLoop:
 					break itemLoop
 				}
 
-				// Check for available IRI on item
+				// Check for available IRI.
 				itemIRI, _ := pub.ToId(next)
 				if itemIRI == nil {
 					continue itemLoop
@@ -354,9 +292,7 @@ stackLoop:
 				//   - any http type error for a new status returns unretrievable
 				_, statusable, _, err := d.getStatusByURI(ctx, username, itemIRI)
 				if err != nil {
-					if !gtserror.IsUnretrievable(err) {
 					l.Errorf("error dereferencing remote status %s: %v", itemIRI, err)
-					}
 					continue itemLoop
 				}
 

internal/federation/federatingdb/create.go

@@ -303,27 +303,9 @@ func (f *federatingDB) createStatusable(
 	statusable ap.Statusable,
 	forwarded bool,
 ) error {
-	// If we do have a forward, we should ignore the content
-	// and instead deref based on the URI of the statusable.
-	//
-	// In other words, don't automatically trust whoever sent
-	// this status to us, but fetch the authentic article from
-	// the server it originated from.
-	if forwarded {
-		// Pass the statusable URI (APIri) into the processor
-		// worker and do the rest of the processing asynchronously.
-		f.state.Workers.EnqueueFediAPI(ctx, messages.FromFediAPI{
-			APObjectType:     ap.ObjectNote,
-			APActivityType:   ap.ActivityCreate,
-			APIri:            ap.GetJSONLDId(statusable),
-			APObjectModel:    nil,
-			GTSModel:         nil,
-			ReceivingAccount: receiver,
-		})
-		return nil
-	}
 
-	// Check whether we should accept this new status.
+	// Check whether we should accept this new status,
+	// we do this BEFORE even handling forwards to us.
 	accept, err := f.shouldAcceptStatusable(ctx,
 		receiver,
 		requester,
@@ -343,6 +325,27 @@ func (f *federatingDB) createStatusable(
 		return nil
 	}
 
+	// If we do have a forward, we should ignore the content
+	// and instead deref based on the URI of the statusable.
+	//
+	// In other words, don't automatically trust whoever sent
+	// this status to us, but fetch the authentic article from
+	// the server it originated from.
+	if forwarded {
+
+		// Pass the statusable URI (APIri) into the processor
+		// worker and do the rest of the processing asynchronously.
+		f.state.Workers.EnqueueFediAPI(ctx, messages.FromFediAPI{
+			APObjectType:     ap.ObjectNote,
+			APActivityType:   ap.ActivityCreate,
+			APIri:            ap.GetJSONLDId(statusable),
+			APObjectModel:    nil,
+			GTSModel:         nil,
+			ReceivingAccount: receiver,
+		})
+		return nil
+	}
+
 	// Do the rest of the processing asynchronously. The processor
 	// will handle inserting/updating + further dereferencing the status.
 	f.state.Workers.EnqueueFediAPI(ctx, messages.FromFediAPI{
@@ -371,13 +374,11 @@ func (f *federatingDB) shouldAcceptStatusable(ctx context.Context, receiver *gts
 		name := mention.NameString
 
 		switch {
-		case accURI != "":
+		case accURI != "" &&
-			if accURI == receiver.URI ||
+			accURI == receiver.URI || accURI == receiver.URL:
-				accURI == receiver.URL {
 			// Mention target is receiver,
 			// they are mentioned in status.
 			return true, nil
-			}
 
 		case accURI == "" && name != "":
 			// Only a name was provided, extract the user@domain parts.

internal/federation/federatingdb/create_test.go

@@ -26,6 +26,8 @@ import (
 	"github.com/superseriousbusiness/activity/streams"
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
+	"github.com/superseriousbusiness/gotosocial/internal/id"
+	"github.com/superseriousbusiness/gotosocial/internal/util"
 )
 
 type CreateTestSuite struct {
@@ -60,7 +62,20 @@ func (suite *CreateTestSuite) TestCreateNoteForward() {
 
 	create := suite.testActivities["forwarded_message"].Activity
 
-	err := suite.federatingDB.Create(ctx, create)
+	// ensure a follow exists between requesting
+	// and receiving account, this ensures the forward
+	// will be seen as "relevant" and not get dropped.
+	err := suite.db.PutFollow(ctx, &gtsmodel.Follow{
+		ID:              id.NewULID(),
+		URI:             "https://this.is.a.url",
+		AccountID:       receivingAccount.ID,
+		TargetAccountID: requestingAccount.ID,
+		ShowReblogs:     util.Ptr(true),
+		Notify:          util.Ptr(false),
+	})
+	suite.NoError(err)
+
+	err = suite.federatingDB.Create(ctx, create)
 	suite.NoError(err)
 
 	// should be a message heading to the processor now, which we can intercept here

internal/visibility/home_timeline.go

@@ -99,10 +99,13 @@ func (f *Filter) isStatusHomeTimelineable(ctx context.Context, owner *gtsmodel.A
 	}
 
 	var (
+		// iterated-over
+		// loop status.
 		next = status
-		oneAuthor = true // Assume one author until proven otherwise.
+
-		included  bool
+		// assume one author
-		converstn bool
+		// until proven otherwise.
+		oneAuthor = true
 	)
 
 	for {
@@ -116,21 +119,28 @@ func (f *Filter) isStatusHomeTimelineable(ctx context.Context, owner *gtsmodel.A
 			next.MentionsAccount(owner.ID) {
 			// Owner is in / mentioned in
 			// this status thread. They can
-			// see all future visible statuses.
+			// see future visible statuses.
-			included = true
+			visible = true
 			break
 		}
 
-		// Check whether this should be a visible conversation, i.e.
+		var notVisible bool
-		// is it between accounts on owner timeline that they follow?
+
-		converstn, err = f.isVisibleConversation(ctx, owner, next)
+		// Check whether status in conversation is explicitly relevant to timeline
+		// owner (i.e. includes mutals), or is explicitly invisible (i.e. blocked).
+		visible, notVisible, err = f.isVisibleConversation(ctx, owner, next)
 		if err != nil {
 			return false, gtserror.Newf("error checking conversation visibility: %w", err)
 		}
 
-		if converstn {
+		if notVisible {
-			// Owner is relevant to this conversation,
+			log.Tracef(ctx, "conversation not visible to timeline owner")
-			// i.e. between follows / mutuals they know.
+			return false, nil
+		}
+
+		if visible {
+			// Conversation relevant
+			// to timeline owner!
 			break
 		}
 
@@ -144,23 +154,23 @@ func (f *Filter) isStatusHomeTimelineable(ctx context.Context, owner *gtsmodel.A
 			break
 		}
 
-		// Fetch next parent in thread.
+		// Check parent is deref'd.
-		parentID := next.InReplyToID
+		if next.InReplyToID == "" {
-		if parentID == "" {
 			log.Warnf(ctx, "status not yet deref'd: %s", next.InReplyToURI)
 			return false, cache.SentinelError
 		}
 
+		// Fetch next parent in conversation.
 		next, err = f.state.DB.GetStatusByID(
 			gtscontext.SetBarebones(ctx),
-			parentID,
+			next.InReplyToID,
 		)
 		if err != nil {
-			return false, gtserror.Newf("error getting status parent %s: %w", parentID, err)
+			return false, gtserror.Newf("error getting status parent %s: %w", next.InReplyToID, err)
 		}
 	}
 
-	if next != status && !oneAuthor && !included && !converstn {
+	if next != status && !oneAuthor && !visible {
 		log.Trace(ctx, "ignoring visible reply in conversation irrelevant to owner")
 		return false, nil
 	}
@@ -193,17 +203,25 @@ func (f *Filter) isStatusHomeTimelineable(ctx context.Context, owner *gtsmodel.A
 	return true, nil
 }
 
-func (f *Filter) isVisibleConversation(ctx context.Context, owner *gtsmodel.Account, status *gtsmodel.Status) (bool, error) {
+func (f *Filter) isVisibleConversation(
+	ctx context.Context,
+	owner *gtsmodel.Account,
+	status *gtsmodel.Status,
+) (
+	bool, // explicitly IS visible
+	bool, // explicitly NOT visible
+	error, // err
+) {
 	// Check if status is visible to the timeline owner.
 	visible, err := f.StatusVisible(ctx, owner, status)
 	if err != nil {
-		return false, err
+		return false, false, err
 	}
 
 	if !visible {
-		// Invisible to
+		// Explicitly NOT visible
-		// timeline owner.
+		// to the timeline owner.
-		return false, nil
+		return false, true, nil
 	}
 
 	if status.Visibility == gtsmodel.VisibilityUnlocked ||
@@ -212,37 +230,50 @@ func (f *Filter) isVisibleConversation(ctx context.Context, owner *gtsmodel.Acco
 		// direct / follow-only / mutual-only visibility statuses
 		// as the above visibility check already handles this.
 
-		// Check if owner follows the status author.
+		// Check owner follows the status author.
-		followAuthor, err := f.state.DB.IsFollowing(ctx,
+		follow, err := f.state.DB.IsFollowing(ctx,
 			owner.ID,
 			status.AccountID,
 		)
 		if err != nil {
-			return false, gtserror.Newf("error checking follow %s->%s: %w", owner.ID, status.AccountID, err)
+			return false, false, gtserror.Newf("error checking follow %s->%s: %w", owner.ID, status.AccountID, err)
 		}
 
-		if !followAuthor {
+		if !follow {
-			// Not a visible status
+			// Not explicitly visible
-			// in conversation thread.
+			// status to timeline owner.
-			return false, nil
+			return false, false, nil
 		}
 	}
 
+	var follow bool
+
 	for _, mention := range status.Mentions {
-		// Check if timeline owner follows target.
+		// Check block between timeline owner and mention.
-		follow, err := f.state.DB.IsFollowing(ctx,
+		block, err := f.state.DB.IsEitherBlocked(ctx,
 			owner.ID,
 			mention.TargetAccountID,
 		)
 		if err != nil {
-			return false, gtserror.Newf("error checking mention follow %s->%s: %w", owner.ID, mention.TargetAccountID, err)
+			return false, false, gtserror.Newf("error checking mention block %s<->%s: %w", owner.ID, mention.TargetAccountID, err)
 		}
 
-		if follow {
+		if block {
-			// Confirmed conversation.
+			// Invisible conversation.
-			return true, nil
+			return false, true, nil
+		}
+
+		if !follow {
+			// See if tl owner follows any of mentions.
+			follow, err = f.state.DB.IsFollowing(ctx,
+				owner.ID,
+				mention.TargetAccountID,
+			)
+			if err != nil {
+				return false, false, gtserror.Newf("error checking mention follow %s->%s: %w", owner.ID, mention.TargetAccountID, err)
+			}
 		}
 	}
 
-	return false, nil
+	return follow, false, nil
 }