Skip to content

WIP: Set backdated timestamp IDs on new old Statuses#5211

Closed
aschmitz wants to merge 1 commit intomastodon:masterfrom
aschmitz:feat/backdate-status-ids-upstream
Closed

WIP: Set backdated timestamp IDs on new old Statuses#5211
aschmitz wants to merge 1 commit intomastodon:masterfrom
aschmitz:feat/backdate-status-ids-upstream

Conversation

@aschmitz
Copy link
Contributor

@aschmitz aschmitz commented Oct 4, 2017

For any "old" status (where the creation time is before the update time, as both are already set by Rails by the time we get to around_save), we now generate an ID for the "old" (or potentially, future-dated) status, and attempt to use that to insert.

In the vast majority of cases, this should succeed. If it doesn't, we retry up to a thousand times, each time incrementing the ID by a random number between 0 and 99, to avoid too many collisions.

Note that this also necessitates setting created_at to nil (or leaving it unset), not Time.utc.now, when creating new statuses. (Although the only downside to setting a time is a small amount of extra work, nothing catastrophic.)

@aschmitz
Copy link
Contributor Author

aschmitz commented Oct 4, 2017

This is based on #4801, and appears to work in some cursory testing, but is marked a WIP because it needs tests.

@Gargron
Copy link
Member

Gargron commented Oct 4, 2017

Time to rebase! I usually do a branch switcharoo with cherry-pick on the latest commit to make things right.

@clworld
Copy link
Contributor

clworld commented Oct 4, 2017

Things I afraid about toot backfillings.

  • Faked publish time (maybe future): If toot with faked publish time (1 year later for example) arrived, that toot stay top of Home & Fedeteted TL (because it will get largest id).
  • Delayed deliver: If remote instance's sidekiq push queue is slow enough, User will get remote instance's toot at N-hour before position (especially on REST API access.)
  • Breaks differential fetch by REST API client: REST API access with since_id = last fetched max status_id will not get newly arrived toots. It's breaking change.

I think normal arriving remote status should be numbered by arriving time and reject futute publish time for backfillings(boosted original status, searched status, importing, etc).

@Gargron
Copy link
Member

Gargron commented Oct 4, 2017

Solutions:

  • Add a flag/option to the processing classes; if true, use local timestamp instead of created_at. Set this flag to true in codepaths that come from ActivityPub::InboxController only (real-time deliveries)
  • If a remote timestamp is in the future, override it with local timestamp

@Gargron Gargron mentioned this pull request Oct 4, 2017
14 tasks
For any "old" status (where the creation time is before the update time,
as both are already set by Rails by the time we get to around_save), we
now generate an ID for the "old" (or potentially, future-dated) status,
and attempt to use that to insert.

In the vast majority of cases, this should succeed. If it doesn't, we
retry up to a thousand times, each time incrementing the ID by a random
number between 0 and 99, to avoid too many collisions.

Note that this also necessitates setting `created_at` to nil (or leaving
it unset), not Time.utc.now, when creating new statuses. (Although the
only downside to setting a time is a small amount of extra work, nothing
catastrophic.)
# repeatedly querying the same set of IDs in such a case.
# (This mostly exists to avoid a DoS attack, and should
# rarely be useful under normal circumstances.)
obj.id = rand(100) if tries.positive?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be +=?

@aschmitz
Copy link
Contributor Author

aschmitz commented Oct 7, 2017

Closing this in favor of #5260.

@aschmitz aschmitz closed this Oct 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants