From e890169e6f151f668580398685c2dbf3c4b780ff Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Fri, 9 Feb 2024 15:24:49 +0100 Subject: [PATCH] use pointer for freshness window (#2614) --- internal/federation/dereferencing/account.go | 104 +++++++++++------- .../federation/dereferencing/dereferencer.go | 38 +++++++ internal/federation/dereferencing/status.go | 62 ++++++----- internal/gtsmodel/status.go | 6 + .../common/{account.go.go => account.go} | 2 +- internal/processing/common/status.go | 39 ++++--- internal/processing/fedi/status.go | 2 +- internal/processing/polls/poll.go | 2 +- internal/processing/status/bookmark.go | 2 +- internal/processing/status/boost.go | 4 +- internal/processing/status/fave.go | 4 +- internal/processing/status/get.go | 6 +- internal/processing/status/mute.go | 2 +- internal/processing/status/pin.go | 2 +- internal/processing/workers/fromfediapi.go | 11 +- 15 files changed, 191 insertions(+), 95 deletions(-) rename internal/processing/common/{account.go.go => account.go} (99%) diff --git a/internal/federation/dereferencing/account.go b/internal/federation/dereferencing/account.go index 86ea9b7fd..7eec6b5b9 100644 --- a/internal/federation/dereferencing/account.go +++ b/internal/federation/dereferencing/account.go @@ -40,41 +40,55 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/util" ) -// accountUpToDate returns whether the given account model is both updateable (i.e. -// non-instance remote account) and whether it needs an update based on `fetched_at`. -func accountUpToDate(account *gtsmodel.Account, force bool) bool { - if !account.SuspendedAt.IsZero() { - // Can't update suspended accounts. - return true +// accountFresh returns true if the given account is +// still considered "fresh" according to the desired +// freshness window (falls back to default if nil). +// +// Local accounts will always be considered fresh because +// there's no remote state that could have changed. +// +// True is also returned for suspended accounts, since +// we'll never want to try to refresh one of these. +// +// Return value of false indicates that the account +// is not fresh and should be refreshed from remote. +func accountFresh( + account *gtsmodel.Account, + window *FreshnessWindow, +) bool { + if window == nil { + window = DefaultAccountFreshness } if account.IsLocal() { - // Can't update local accounts. + // Can't refresh + // local accounts. return true } - if account.IsInstance() && !account.IsNew() { - // Existing instance account. No need for update. + if !account.SuspendedAt.IsZero() { + // Can't refresh + // suspended accounts. return true } - // Default limit we allow - // statuses to be refreshed. - limit := 6 * 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 account was updated recently (within limit), we return as-is. - if next := account.FetchedAt.Add(limit); time.Now().Before(next) { + if account.IsInstance() && + !account.IsNew() { + // Existing instance account. + // No need for refresh. return true } - return false + // Moment when the account is + // considered stale according to + // desired freshness window. + staleAt := account.FetchedAt.Add( + time.Duration(*window), + ) + + // It's still fresh if the time now + // is not past the point of staleness. + return !time.Now().After(staleAt) } // GetAccountByURI will attempt to fetch an accounts by its URI, first checking the database. In the case of a newly-met remote model, or a remote model @@ -146,7 +160,7 @@ func (d *Dereferencer) getAccountByURI(ctx context.Context, requestUser string, }, nil) } - if accountUpToDate(account, false) { + if accountFresh(account, nil) { // This is an existing account that is up-to-date, // before returning ensure it is fully populated. if err := d.state.DB.PopulateAccount(ctx, account); err != nil { @@ -248,7 +262,7 @@ func (d *Dereferencer) getAccountByUsernameDomain( requestUser, account, nil, - false, + nil, ) if err != nil { // Fallback to existing. @@ -265,22 +279,28 @@ func (d *Dereferencer) getAccountByUsernameDomain( return latest, accountable, nil } -// RefreshAccount updates the given account if remote and last_fetched is -// beyond fetch interval, or if force is set. An updated account model is -// returned, but in the case of dereferencing, some low-priority account info -// may be enqueued for asynchronous fetching, e.g. featured account statuses (pins). -// An ActivityPub object indicates the account was dereferenced (i.e. updated). +// RefreshAccount updates the given account if it's a +// remote account, and considered stale / not fresh +// based on Account.FetchedAt and desired freshness. +// +// An updated account model is returned, but in the +// case of dereferencing, some low-priority account +// info may be enqueued for asynchronous fetching, +// e.g. featured account statuses (pins). +// +// An ActivityPub object indicates the account was +// dereferenced (i.e. updated). func (d *Dereferencer) RefreshAccount( ctx context.Context, requestUser string, account *gtsmodel.Account, accountable ap.Accountable, - force bool, + window *FreshnessWindow, ) (*gtsmodel.Account, ap.Accountable, error) { // If no incoming data is provided, - // check whether account needs update. + // check whether account needs refresh. if accountable == nil && - accountUpToDate(account, force) { + accountFresh(account, window) { return account, nil, nil } @@ -314,21 +334,25 @@ func (d *Dereferencer) RefreshAccount( return latest, accountable, nil } -// RefreshAccountAsync enqueues the given account for an asychronous -// update fetching, if last_fetched is beyond fetch interval, or if force -// is set. This is a more optimized form of manually enqueueing .UpdateAccount() -// to the federation worker, since it only enqueues update if necessary. +// RefreshAccountAsync enqueues the given account for +// an asychronous update fetching, if it's a remote +// account, and considered stale / not fresh based on +// Account.FetchedAt and desired freshness. +// +// This is a more optimized form of manually enqueueing +// .UpdateAccount() to the federation worker, since it +// only enqueues update if necessary. func (d *Dereferencer) RefreshAccountAsync( ctx context.Context, requestUser string, account *gtsmodel.Account, accountable ap.Accountable, - force bool, + window *FreshnessWindow, ) { // If no incoming data is provided, - // check whether account needs update. + // check whether account needs refresh. if accountable == nil && - accountUpToDate(account, force) { + accountFresh(account, window) { return } diff --git a/internal/federation/dereferencing/dereferencer.go b/internal/federation/dereferencing/dereferencer.go index 5bd16c1e0..f4596935e 100644 --- a/internal/federation/dereferencing/dereferencer.go +++ b/internal/federation/dereferencing/dereferencer.go @@ -20,11 +20,49 @@ package dereferencing import ( "net/url" "sync" + "time" "github.com/superseriousbusiness/gotosocial/internal/media" "github.com/superseriousbusiness/gotosocial/internal/state" "github.com/superseriousbusiness/gotosocial/internal/transport" "github.com/superseriousbusiness/gotosocial/internal/typeutils" + "github.com/superseriousbusiness/gotosocial/internal/util" +) + +// FreshnessWindow represents a duration in which a +// Status or Account is still considered to be "fresh" +// (ie., not in need of a refresh from remote), if its +// last FetchedAt value falls within the window. +// +// For example, if an Account was FetchedAt 09:00, and it +// is now 12:00, then it would be considered "fresh" +// according to DefaultAccountFreshness, but not according +// to Fresh, which would indicate that the Account requires +// refreshing from remote. +type FreshnessWindow time.Duration + +var ( + // 6 hours. + // + // Default window for doing a + // fresh dereference of an Account. + DefaultAccountFreshness = util.Ptr(FreshnessWindow(6 * time.Hour)) + + // 2 hours. + // + // Default window for doing a + // fresh dereference of a Status. + DefaultStatusFreshness = util.Ptr(FreshnessWindow(2 * time.Hour)) + + // 5 minutes. + // + // Fresh is useful when you're wanting + // a more up-to-date model of something + // that exceeds default freshness windows. + // + // This is tuned to be quite fresh without + // causing loads of dereferencing calls. + Fresh = util.Ptr(FreshnessWindow(5 * time.Minute)) ) // Dereferencer wraps logic and functionality for doing dereferencing diff --git a/internal/federation/dereferencing/status.go b/internal/federation/dereferencing/status.go index 71cc5c530..23c6e98c8 100644 --- a/internal/federation/dereferencing/status.go +++ b/internal/federation/dereferencing/status.go @@ -38,31 +38,41 @@ import ( "github.com/superseriousbusiness/gotosocial/internal/util" ) -// 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 != nil && *status.Local { - // Can't update local statuses. +// statusFresh returns true if the given status is still +// considered "fresh" according to the desired freshness +// window (falls back to default status freshness if nil). +// +// Local statuses will always be considered fresh, +// because there's no remote state that may have changed. +// +// Return value of false indicates that the status +// is not fresh and should be refreshed from remote. +func statusFresh( + status *gtsmodel.Status, + window *FreshnessWindow, +) bool { + // Take default if no + // freshness window preferred. + if window == nil { + window = DefaultStatusFreshness + } + + if status.IsLocal() { + // Can't refresh + // local statuses. return true } - // Default limit we allow - // statuses to be refreshed. - limit := 2 * time.Hour + // Moment when the status is + // considered stale according to + // desired freshness window. + staleAt := status.FetchedAt.Add( + time.Duration(*window), + ) - 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 - } - - return false + // It's still fresh if the time now + // is not past the point of staleness. + return !time.Now().After(staleAt) } // GetStatusByURI will attempt to fetch a status by its URI, first checking the database. In the case of a newly-met remote model, or a remote model whose 'last_fetched' date @@ -146,7 +156,7 @@ func (d *Dereferencer) getStatusByURI(ctx context.Context, requestUser string, u }, nil) } - if statusUpToDate(status, false) { + if statusFresh(status, DefaultStatusFreshness) { // This is an existing status that is up-to-date, // before returning ensure it is fully populated. if err := d.state.DB.PopulateStatus(ctx, status); err != nil { @@ -181,12 +191,12 @@ func (d *Dereferencer) RefreshStatus( requestUser string, status *gtsmodel.Status, statusable ap.Statusable, - force bool, + window *FreshnessWindow, ) (*gtsmodel.Status, ap.Statusable, error) { // If no incoming data is provided, // check whether status needs update. if statusable == nil && - statusUpToDate(status, force) { + statusFresh(status, window) { return status, nil, nil } @@ -228,12 +238,12 @@ func (d *Dereferencer) RefreshStatusAsync( requestUser string, status *gtsmodel.Status, statusable ap.Statusable, - force bool, + window *FreshnessWindow, ) { // If no incoming data is provided, // check whether status needs update. if statusable == nil && - statusUpToDate(status, force) { + statusFresh(status, window) { return } diff --git a/internal/gtsmodel/status.go b/internal/gtsmodel/status.go index 79c67c933..3bbe82c08 100644 --- a/internal/gtsmodel/status.go +++ b/internal/gtsmodel/status.go @@ -205,6 +205,12 @@ func (s *Status) BelongsToAccount(accountID string) bool { return s.AccountID == accountID } +// IsLocal returns true if this is a local +// status (ie., originating from this instance). +func (s *Status) IsLocal() bool { + return s.Local != nil && *s.Local +} + // StatusToTag is an intermediate struct to facilitate the many2many relationship between a status and one or more tags. type StatusToTag struct { StatusID string `bun:"type:CHAR(26),unique:statustag,nullzero,notnull"` diff --git a/internal/processing/common/account.go.go b/internal/processing/common/account.go similarity index 99% rename from internal/processing/common/account.go.go rename to internal/processing/common/account.go index f4bd06e76..9a39ea26d 100644 --- a/internal/processing/common/account.go.go +++ b/internal/processing/common/account.go @@ -66,7 +66,7 @@ func (p *Processor) GetTargetAccountBy( requester.Username, target, nil, - false, + nil, ) } diff --git a/internal/processing/common/status.go b/internal/processing/common/status.go index ae03a5306..308f5173f 100644 --- a/internal/processing/common/status.go +++ b/internal/processing/common/status.go @@ -23,19 +23,24 @@ import ( apimodel "github.com/superseriousbusiness/gotosocial/internal/api/model" "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing" "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/log" ) -// 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. +// 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. +// +// window can be used to force refresh of the target if it's +// deemed to be stale. Falls back to default window if nil. func (p *Processor) GetTargetStatusBy( ctx context.Context, requester *gtsmodel.Account, getTargetFromDB func() (*gtsmodel.Status, error), - refresh bool, + window *dereferencing.FreshnessWindow, ) ( status *gtsmodel.Status, visible bool, @@ -68,13 +73,15 @@ func (p *Processor) GetTargetStatusBy( // a requester (i.e. request is authorized) // to prevent a possible DOS vector. - if refresh { - // Refresh required, forcibly do synchronously. + if window != nil { + // Window is explicitly set, so likely + // tighter than the default window. + // Do refresh synchronously. _, _, err := p.federator.RefreshStatus(ctx, requester.Username, target, nil, - true, // force + window, ) if err != nil { log.Errorf(ctx, "error refreshing status: %v", err) @@ -85,7 +92,7 @@ func (p *Processor) GetTargetStatusBy( requester.Username, target, nil, - false, // force + nil, ) } } @@ -95,11 +102,14 @@ func (p *Processor) GetTargetStatusBy( // GetVisibleTargetStatus calls GetTargetStatusBy(), // but converts a non-visible result to not-found error. +// +// window can be used to force refresh of the target if it's +// deemed to be stale. Falls back to default window if nil. func (p *Processor) GetVisibleTargetStatusBy( ctx context.Context, requester *gtsmodel.Account, getTargetFromDB func() (*gtsmodel.Status, error), - refresh bool, + window *dereferencing.FreshnessWindow, ) ( status *gtsmodel.Status, errWithCode gtserror.WithCode, @@ -108,7 +118,7 @@ func (p *Processor) GetVisibleTargetStatusBy( target, visible, errWithCode := p.GetTargetStatusBy(ctx, requester, getTargetFromDB, - refresh, + window, ) if errWithCode != nil { return nil, errWithCode @@ -128,18 +138,21 @@ func (p *Processor) GetVisibleTargetStatusBy( // GetVisibleTargetStatus calls GetVisibleTargetStatusBy(), // passing in a database function that fetches by status ID. +// +// window can be used to force refresh of the target if it's +// deemed to be stale. Falls back to default window if nil. func (p *Processor) GetVisibleTargetStatus( ctx context.Context, requester *gtsmodel.Account, targetID string, - refresh bool, + window *dereferencing.FreshnessWindow, ) ( status *gtsmodel.Status, errWithCode gtserror.WithCode, ) { return p.GetVisibleTargetStatusBy(ctx, requester, func() (*gtsmodel.Status, error) { return p.state.DB.GetStatusByID(ctx, targetID) - }, refresh) + }, window) } // UnwrapIfBoost "unwraps" the given status if @@ -158,7 +171,7 @@ func (p *Processor) UnwrapIfBoost( return p.GetVisibleTargetStatus(ctx, requester, status.BoostOfID, - false, + nil, ) } diff --git a/internal/processing/fedi/status.go b/internal/processing/fedi/status.go index 2674ebf68..2849d08a4 100644 --- a/internal/processing/fedi/status.go +++ b/internal/processing/fedi/status.go @@ -100,7 +100,7 @@ func (p *Processor) StatusRepliesGet( status, errWithCode := p.c.GetVisibleTargetStatus(ctx, requester, statusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode diff --git a/internal/processing/polls/poll.go b/internal/processing/polls/poll.go index 19cf555e5..fe8fc71c5 100644 --- a/internal/processing/polls/poll.go +++ b/internal/processing/polls/poll.go @@ -53,7 +53,7 @@ func (p *Processor) getTargetPoll(ctx context.Context, requester *gtsmodel.Accou func() (*gtsmodel.Status, error) { return p.state.DB.GetStatusByPollID(ctx, targetID) }, - true, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode diff --git a/internal/processing/status/bookmark.go b/internal/processing/status/bookmark.go index 224445838..778492c71 100644 --- a/internal/processing/status/bookmark.go +++ b/internal/processing/status/bookmark.go @@ -33,7 +33,7 @@ func (p *Processor) getBookmarkableStatus(ctx context.Context, requestingAccount targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, "", errWithCode diff --git a/internal/processing/status/boost.go b/internal/processing/status/boost.go index 2fc96091e..5fae695fd 100644 --- a/internal/processing/status/boost.go +++ b/internal/processing/status/boost.go @@ -43,7 +43,7 @@ func (p *Processor) BoostCreate( ctx, requester, targetID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode @@ -113,7 +113,7 @@ func (p *Processor) BoostRemove( ctx, requester, targetID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode diff --git a/internal/processing/status/fave.go b/internal/processing/status/fave.go index 7ac270e8c..7b71725ab 100644 --- a/internal/processing/status/fave.go +++ b/internal/processing/status/fave.go @@ -47,7 +47,7 @@ func (p *Processor) getFaveableStatus( ctx, requester, targetID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, nil, errWithCode @@ -153,7 +153,7 @@ func (p *Processor) FavedBy(ctx context.Context, requestingAccount *gtsmodel.Acc targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode diff --git a/internal/processing/status/get.go b/internal/processing/status/get.go index 7c275acbd..475ab0128 100644 --- a/internal/processing/status/get.go +++ b/internal/processing/status/get.go @@ -32,7 +32,7 @@ func (p *Processor) Get(ctx context.Context, requestingAccount *gtsmodel.Account targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode @@ -46,7 +46,7 @@ func (p *Processor) WebGet(ctx context.Context, targetStatusID string) (*apimode targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, nil, // requester targetStatusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode @@ -69,7 +69,7 @@ func (p *Processor) contextGet( targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode diff --git a/internal/processing/status/mute.go b/internal/processing/status/mute.go index fb4f3b384..8888b59d4 100644 --- a/internal/processing/status/mute.go +++ b/internal/processing/status/mute.go @@ -44,7 +44,7 @@ func (p *Processor) getMuteableStatus( targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode diff --git a/internal/processing/status/pin.go b/internal/processing/status/pin.go index f08b9652c..9a4a4b266 100644 --- a/internal/processing/status/pin.go +++ b/internal/processing/status/pin.go @@ -42,7 +42,7 @@ func (p *Processor) getPinnableStatus(ctx context.Context, requestingAccount *gt targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID, - false, // refresh + nil, // default freshness ) if errWithCode != nil { return nil, errWithCode diff --git a/internal/processing/workers/fromfediapi.go b/internal/processing/workers/fromfediapi.go index 6dd4e543d..74ec0db25 100644 --- a/internal/processing/workers/fromfediapi.go +++ b/internal/processing/workers/fromfediapi.go @@ -23,6 +23,8 @@ import ( "codeberg.org/gruf/go-kv" "codeberg.org/gruf/go-logger/v2/level" "github.com/superseriousbusiness/gotosocial/internal/ap" + "github.com/superseriousbusiness/gotosocial/internal/federation/dereferencing" + "github.com/superseriousbusiness/gotosocial/internal/gtscontext" "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" @@ -179,7 +181,8 @@ func (p *fediAPI) CreateStatus(ctx context.Context, fMsg messages.FromFediAPI) e fMsg.ReceivingAccount.Username, bareStatus, statusable, - true, + // Force refresh within 5min window. + dereferencing.Fresh, ) if err != nil { return gtserror.Newf("error processing new status %s: %w", bareStatus.URI, err) @@ -487,7 +490,8 @@ func (p *fediAPI) UpdateAccount(ctx context.Context, fMsg messages.FromFediAPI) fMsg.ReceivingAccount.Username, account, apubAcc, - true, // Force refresh. + // Force refresh within 5min window. + dereferencing.Fresh, ) if err != nil { log.Errorf(ctx, "error refreshing account: %v", err) @@ -512,7 +516,8 @@ func (p *fediAPI) UpdateStatus(ctx context.Context, fMsg messages.FromFediAPI) e fMsg.ReceivingAccount.Username, existing, apStatus, - true, + // Force refresh within 5min window. + dereferencing.Fresh, ) if err != nil { log.Errorf(ctx, "error refreshing status: %v", err)