[bugfix] use a much shorter refresh limit for statuses with polls (#2453

)

* specifically use a much shorter refresh limit for statuses with polls

* allow specifying whether status must be upToDate in calls to Get(Visible)?TargetStatusBy_(), limit force refresh to 5 minute cooldown

* remove the PollID check from statusUpToDate()

* remove unnecessary force flag checks

* remove unused field

* check refresh status error

* use argument name 'refresh' instead of 'upToDate' to better fit with the codebase

* add statuses_poll_id_idx

* remove the definitely-not copy-pasted comment i accidentally typed out in full

* only synchronously refresh if the refresh flag is provided, otherwise do async

* fix wrong force value being provided for async

---------

Co-authored-by: tobi <tobi.smethurst@protonmail.com>

internal/cache/gts.go

@@ -952,6 +952,7 @@ func (c *GTSCaches) initStatus() {
 		{Name: "ID"},
 		{Name: "URI"},
 		{Name: "URL"},
+		{Name: "PollID"},
 		{Name: "BoostOfID.AccountID"},
 		{Name: "ThreadID", Multi: true},
 	}, copyF, cap)

internal/db/bundb/migrations/20231215115920_add_status_poll_index.go

@@ -0,0 +1,66 @@
+// GoToSocial
+// Copyright (C) GoToSocial Authors admin@gotosocial.org
+// SPDX-License-Identifier: AGPL-3.0-or-later
+//
+// This program is free software: you can redistribute it and/or modify
+// it under the terms of the GNU Affero General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// This program is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU Affero General Public License for more details.
+//
+// You should have received a copy of the GNU Affero General Public License
+// along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+package migrations
+
+import (
+	"context"
+
+	"github.com/uptrace/bun"
+)
+
+func init() {
+	up := func(ctx context.Context, db *bun.DB) error {
+		return db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error {
+			type spec struct {
+				index   string
+				table   string
+				columns []string
+			}
+
+			for _, spec := range []spec{
+				{
+					index:   "statuses_poll_id_idx",
+					table:   "statuses",
+					columns: []string{"poll_id"},
+				},
+			} {
+				if _, err := tx.
+					NewCreateIndex().
+					Table(spec.table).
+					Index(spec.index).
+					Column(spec.columns...).
+					IfNotExists().
+					Exec(ctx); err != nil {
+					return err
+				}
+			}
+
+			return nil
+		})
+	}
+
+	down := func(ctx context.Context, db *bun.DB) error {
+		return db.RunInTx(ctx, nil, func(ctx context.Context, tx bun.Tx) error {
+			return nil
+		})
+	}
+
+	if err := Migrations.Register(up, down); err != nil {
+		panic(err)
+	}
+}

internal/db/bundb/poll.go

@@ -50,20 +50,6 @@ func (p *pollDB) GetPollByID(ctx context.Context, id string) (*gtsmodel.Poll, er
 	)
 }
 
-func (p *pollDB) GetPollByStatusID(ctx context.Context, statusID string) (*gtsmodel.Poll, error) {
-	return p.getPoll(
-		ctx,
-		"StatusID",
-		func(poll *gtsmodel.Poll) error {
-			return p.db.NewSelect().
-				Model(poll).
-				Where("? = ?", bun.Ident("poll.status_id"), statusID).
-				Scan(ctx)
-		},
-		statusID,
-	)
-}
-
 func (p *pollDB) getPoll(ctx context.Context, lookup string, dbQuery func(*gtsmodel.Poll) error, keyParts ...any) (*gtsmodel.Poll, error) {
 	// Fetch poll from database cache with loader callback
 	poll, err := p.state.Caches.GTS.Poll().Load(lookup, func() (*gtsmodel.Poll, error) {

internal/db/bundb/poll_test.go

@@ -67,10 +67,6 @@ func (suite *PollTestSuite) TestGetPollBy() {
 			"id": func() (*gtsmodel.Poll, error) {
 				return suite.db.GetPollByID(ctx, poll.ID)
 			},
-
-			"status_id": func() (*gtsmodel.Poll, error) {
-				return suite.db.GetPollByStatusID(ctx, poll.StatusID)
-			},
 		} {
 
 			// Clear database caches.
@@ -287,10 +283,6 @@ func (suite *PollTestSuite) TestDeletePoll() {
 		// Ensure that afterwards we cannot fetch poll.
 		_, err = suite.db.GetPollByID(ctx, poll.ID)
 		suite.ErrorIs(err, db.ErrNoEntries)
-
-		// Or again by the status it's attached to.
-		_, err = suite.db.GetPollByStatusID(ctx, poll.StatusID)
-		suite.ErrorIs(err, db.ErrNoEntries)
 	}
 }
 

internal/db/bundb/status.go

@@ -87,6 +87,17 @@ func (s *statusDB) GetStatusByURL(ctx context.Context, url string) (*gtsmodel.St
 	)
 }
 
+func (s *statusDB) GetStatusByPollID(ctx context.Context, pollID string) (*gtsmodel.Status, error) {
+	return s.getStatus(
+		ctx,
+		"PollID",
+		func(status *gtsmodel.Status) error {
+			return s.db.NewSelect().Model(status).Where("? = ?", bun.Ident("status.poll_id"), pollID).Scan(ctx)
+		},
+		pollID,
+	)
+}
+
 func (s *statusDB) GetStatusBoost(ctx context.Context, boostOfID string, byAccountID string) (*gtsmodel.Status, error) {
 	return s.getStatus(
 		ctx,

internal/db/poll.go

@@ -27,9 +27,6 @@ type Poll interface {
 	// GetPollByID fetches the Poll with given ID from the database.
 	GetPollByID(ctx context.Context, id string) (*gtsmodel.Poll, error)
 
-	// GetPollByStatusID fetches the Poll with given status ID column value from the database.
-	GetPollByStatusID(ctx context.Context, statusID string) (*gtsmodel.Poll, error)
-
 	// GetOpenPolls fetches all local Polls in the database with an unset `closed_at` column.
 	GetOpenPolls(ctx context.Context) ([]*gtsmodel.Poll, error)
 

internal/db/status.go

@@ -25,15 +25,18 @@ import (
 
 // Status contains functions for getting statuses, creating statuses, and checking various other fields on statuses.
 type Status interface {
-	// GetStatusByID returns one status from the database, with no rel fields populated, only their linking ID / URIs
+	// GetStatusByID fetches the status from the database with matching id column.
 	GetStatusByID(ctx context.Context, id string) (*gtsmodel.Status, error)
 
-	// GetStatusByURI returns one status from the database, with no rel fields populated, only their linking ID / URIs
+	// GetStatusByURI fetches the status from the database with matching uri column.
 	GetStatusByURI(ctx context.Context, uri string) (*gtsmodel.Status, error)
 
-	// GetStatusByURL returns one status from the database, with no rel fields populated, only their linking ID / URIs
+	// GetStatusByURL fetches the status from the database with matching url column.
 	GetStatusByURL(ctx context.Context, uri string) (*gtsmodel.Status, error)
 
+	// GetStatusByPollID fetches the status from the database with matching poll_id column.
+	GetStatusByPollID(ctx context.Context, pollID string) (*gtsmodel.Status, error)
+
 	// GetStatusBoost fetches the status whose boost_of_id column refers to boostOfID, authored by given account ID.
 	GetStatusBoost(ctx context.Context, boostOfID string, byAccountID string) (*gtsmodel.Status, error)
 

internal/federation/dereferencing/status.go

@@ -40,14 +40,25 @@ 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) bool {
+func statusUpToDate(status *gtsmodel.Status, force bool) bool {
 	if *status.Local {
 		// Can't update local statuses.
 		return true
 	}
 
-	// If this status was updated recently (last interval), we return as-is.
-	if next := status.FetchedAt.Add(2 * time.Hour); time.Now().Before(next) {
+	// Default limit we allow
+	// statuses to be refreshed.
+	limit := 2 * time.Hour
+
+	if force {
+		// We specifically allow the force flag
+		// to force an early refresh (on a much
+		// smaller cooldown period).
+		limit = 5 * time.Minute
+	}
+
+	// If this status was updated recently (within limit), return as-is.
+	if next := status.FetchedAt.Add(limit); time.Now().Before(next) {
 		return true
 	}
 
@@ -125,7 +136,7 @@ func (d *Dereferencer) getStatusByURI(ctx context.Context, requestUser string, u
 	}
 
 	// Check whether needs update.
-	if statusUpToDate(status) {
+	if statusUpToDate(status, false) {
 		// This is existing up-to-date status, ensure it is populated.
 		if err := d.state.DB.PopulateStatus(ctx, status); err != nil {
 			log.Errorf(ctx, "error populating existing status: %v", err)
@@ -159,8 +170,8 @@ func (d *Dereferencer) RefreshStatus(
 	statusable ap.Statusable,
 	force bool,
 ) (*gtsmodel.Status, ap.Statusable, error) {
-	// Check whether needs update.
-	if !force && statusUpToDate(status) {
+	// Check whether status needs update.
+	if statusUpToDate(status, force) {
 		return status, nil, nil
 	}
 
@@ -204,8 +215,8 @@ func (d *Dereferencer) RefreshStatusAsync(
 	statusable ap.Statusable,
 	force bool,
 ) {
-	// Check whether needs update.
-	if !force && statusUpToDate(status) {
+	// Check whether status needs update.
+	if statusUpToDate(status, force) {
 		return
 	}
 

internal/processing/common/status.go

@@ -30,10 +30,12 @@ import (
 
 // GetTargetStatusBy fetches the target status with db load function, given the authorized (or, nil) requester's
 // account. This returns an approprate gtserror.WithCode accounting for not found and visibility to requester.
+// The refresh argument allows specifying whether the returned copy should be force refreshed.
 func (p *Processor) GetTargetStatusBy(
 	ctx context.Context,
 	requester *gtsmodel.Account,
 	getTargetFromDB func() (*gtsmodel.Status, error),
+	refresh bool,
 ) (
 	status *gtsmodel.Status,
 	visible bool,
@@ -61,47 +63,52 @@ func (p *Processor) GetTargetStatusBy(
 	}
 
 	if requester != nil && visible {
-		// Ensure remote status is up-to-date.
-		p.federator.RefreshStatusAsync(ctx,
-			requester.Username,
-			target,
-			nil,
-			false,
-		)
+		// We only bother refreshing if this status
+		// is visible to requester, AND there *is*
+		// a requester (i.e. request is authorized)
+		// to prevent a possible DOS vector.
+
+		if refresh {
+			// Refresh required, forcibly do synchronously.
+			_, _, err := p.federator.RefreshStatus(ctx,
+				requester.Username,
+				target,
+				nil,
+				true, // force
+			)
+			if err != nil {
+				log.Errorf(ctx, "error refreshing status: %v", err)
+			}
+		} else {
+			// Only refresh async *if* out-of-date.
+			p.federator.RefreshStatusAsync(ctx,
+				requester.Username,
+				target,
+				nil,
+				false, // force
+			)
+		}
 	}
 
 	return target, visible, nil
 }
 
-// GetTargetStatusByID is a call-through to GetTargetStatus() using the db GetStatusByID() function.
-func (p *Processor) GetTargetStatusByID(
-	ctx context.Context,
-	requester *gtsmodel.Account,
-	targetID string,
-) (
-	status *gtsmodel.Status,
-	visible bool,
-	errWithCode gtserror.WithCode,
-) {
-	return p.GetTargetStatusBy(ctx, requester, func() (*gtsmodel.Status, error) {
-		return p.state.DB.GetStatusByID(ctx, targetID)
-	})
-}
-
-// GetVisibleTargetStatus calls GetTargetStatusByID(),
+// GetVisibleTargetStatus calls GetTargetStatusBy(),
 // but converts a non-visible result to not-found error.
-func (p *Processor) GetVisibleTargetStatus(
+func (p *Processor) GetVisibleTargetStatusBy(
 	ctx context.Context,
 	requester *gtsmodel.Account,
-	targetID string,
+	getTargetFromDB func() (*gtsmodel.Status, error),
+	refresh bool,
 ) (
 	status *gtsmodel.Status,
 	errWithCode gtserror.WithCode,
 ) {
 	// Fetch the target status by ID from the database.
-	target, visible, errWithCode := p.GetTargetStatusByID(ctx,
+	target, visible, errWithCode := p.GetTargetStatusBy(ctx,
 		requester,
-		targetID,
+		getTargetFromDB,
+		refresh,
 	)
 	if errWithCode != nil {
 		return nil, errWithCode
@@ -119,6 +126,22 @@ func (p *Processor) GetVisibleTargetStatus(
 	return target, nil
 }
 
+// GetVisibleTargetStatus calls GetVisibleTargetStatusBy(),
+// passing in a database function that fetches by status ID.
+func (p *Processor) GetVisibleTargetStatus(
+	ctx context.Context,
+	requester *gtsmodel.Account,
+	targetID string,
+	refresh bool,
+) (
+	status *gtsmodel.Status,
+	errWithCode gtserror.WithCode,
+) {
+	return p.GetVisibleTargetStatusBy(ctx, requester, func() (*gtsmodel.Status, error) {
+		return p.state.DB.GetStatusByID(ctx, targetID)
+	}, refresh)
+}
+
 // UnwrapIfBoost "unwraps" the given status if
 // it's a boost wrapper, by returning the boosted
 // status it targets (pending visibility checks).
@@ -132,9 +155,10 @@ func (p *Processor) UnwrapIfBoost(
 	if status.BoostOfID == "" {
 		return status, nil
 	}
-
 	return p.GetVisibleTargetStatus(ctx,
-		requester, status.BoostOfID,
+		requester,
+		status.BoostOfID,
+		false,
 	)
 }
 

internal/processing/fedi/status.go

@@ -100,6 +100,7 @@ func (p *Processor) StatusRepliesGet(
 	status, errWithCode := p.c.GetVisibleTargetStatus(ctx,
 		requester,
 		statusID,
+		false, // refresh
 	)
 	if errWithCode != nil {
 		return nil, errWithCode