Skip to content

Commit

Permalink
drop per-target delivery tracking
Browse files Browse the repository at this point in the history 
sad, it's useful, but it's too expensive. for #1501, #1149
  • Loading branch information
@snarfed
snarfed committed Dec 15, 2024
1 parent 604a7f6 commit 63c0e25
Show file tree
Hide file tree
Showing 11 changed files with 36 additions and 265 deletions.
2 changes: 1 addition & 1 deletion dms.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ def maybe_send(*, from_proto, to_user, text, type=None, in_reply_to=None):
id = f'{bot.profile_id()}#{type or "?"}-dm-{to_user.key.id()}-{util.now().isoformat()}'
target_uri = to_user.target_for(to_user.obj, shared=False)
target = models.Target(protocol=to_user.LABEL, uri=target_uri)
models.Object(id=id, source_protocol='web', undelivered=[target], our_as1={
models.Object(id=id, source_protocol='web', our_as1={
'objectType': 'activity',
'verb': 'post',
'id': f'{id}-create',
Expand Down
14 changes: 8 additions & 6 deletions models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -875,7 +875,6 @@ class Object(StringIdModel):
Key name is the id. We synthesize ids if necessary.
"""
STATUSES = ('new', 'in progress', 'complete', 'failed', 'ignored')
LABELS = ('activity',
# DEPRECATED, replaced by users, notify, feed
'feed', 'notification', 'user')
Expand All @@ -893,7 +892,6 @@ class Object(StringIdModel):
# with old Objects in the datastore that we haven't bothered migrating.
domains = ndb.StringProperty(repeated=True)

status = ndb.StringProperty(choices=STATUSES)
# choices is populated in reset_protocol_properties, after all User
# subclasses are created, so that PROTOCOLS is fully populated.
# TODO: nail down whether this is ABBREV or LABEL
Expand All @@ -918,10 +916,6 @@ class Object(StringIdModel):

deleted = ndb.BooleanProperty()

delivered = ndb.StructuredProperty(Target, repeated=True)
undelivered = ndb.StructuredProperty(Target, repeated=True)
failed = ndb.StructuredProperty(Target, repeated=True)

# Copies of this object elsewhere, eg at:// URIs for ATProto records and
# nevent etc bech32-encoded Nostr ids, where this object is the original.
# Similar to u-syndication links in microformats2 and
Expand All @@ -944,6 +938,14 @@ class Object(StringIdModel):
lock = None
"""Initialized in __init__, synchronizes :meth:`add` and :meth:`remove`."""

# these were used for delivery tracking, but they were too expensive,
# so we stopped: https://github.com/snarfed/bridgy-fed/issues/1501
STATUSES = ('new', 'in progress', 'complete', 'failed', 'ignored')
status = ndb.StringProperty(choices=STATUSES)
delivered = ndb.StructuredProperty(Target, repeated=True)
undelivered = ndb.StructuredProperty(Target, repeated=True)
failed = ndb.StructuredProperty(Target, repeated=True)

@property
def as1(self):
def use_urls_as_ids(obj):
Expand Down
55 changes: 7 additions & 48 deletions protocol.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1138,16 +1138,14 @@ def maybe_accept_follow(_, follower, followee, follow):
# send accept. note that this is one accept for the whole
# follow, even if it has multiple followees!
id = f'{followee.key.id()}/followers#accept-{follow.key.id()}'
undelivered = [Target(protocol=follower.LABEL, uri=target)]
accept = {
'id': id,
'objectType': 'activity',
'verb': 'accept',
'actor': followee.key.id(),
'object': follow.as1,
}
Object.get_or_create(id, authed_as=followee.key.id(),
undelivered=undelivered, our_as1=accept)
Object.get_or_create(id, authed_as=followee.key.id(), our_as1=accept)

common.create_task(queue='send', obj_id=id, url=target,
protocol=follower.LABEL, user=followee.key.urlsafe())
Expand All @@ -1174,7 +1172,6 @@ def bot_follow(bot_cls, user):
target = user.target_for(user.obj)
follow_back_id = f'https://{bot.key.id()}/#follow-back-{user.key.id()}-{now}'
Object(id=follow_back_id, source_protocol='web',
undelivered=[Target(protocol=user.LABEL, uri=target)],
our_as1={
'objectType': 'activity',
'verb': 'follow',
Expand Down Expand Up @@ -1234,11 +1231,11 @@ def handle_bare_object(cls, obj, authed_as=None):

create_id = f'{obj.key.id()}#bridgy-fed-create'
create = cls.load(create_id, remote=False)
if (obj.new or not create or create.status != 'complete'
if (obj.new or not create
# HACK: force query param here is specific to webmention
or 'force' in request.form):
if create:
logger.info(f'Existing create {create.key.id()} status {create.status}')
logger.info(f'Existing create {create.key.id()}')
else:
logger.info(f'No existing create activity')
create_as1 = {
Expand Down Expand Up @@ -1276,21 +1273,15 @@ def deliver(from_cls, obj, from_user, to_proto=None):
# find delivery targets. maps Target to Object or None
targets = from_cls.targets(obj, from_user=from_user)

# TODO: this would be clearer if it was at the end of receive(), which
# that *should* be equivalent, but oddly tests fail if it's moved there
obj.put()
if not targets:
obj.status = 'ignored'
obj.put()
return r'No targets, nothing to do ¯\_(ツ)_/¯', 204

# sort targets so order is deterministic for tests, debugging, etc
sorted_targets = sorted(targets.items(), key=lambda t: t[0].uri)
obj.populate(
status='in progress',
delivered=[],
failed=[],
undelivered=[t for t, _ in sorted_targets],
)
obj.put()
logger.info(f'Delivering to: {obj.undelivered}')
logger.info(f'Delivering to: {[t for t, _ in sorted_targets]}')

# enqueue send task for each targets
user = from_user.key.urlsafe()
Expand Down Expand Up @@ -1749,11 +1740,6 @@ def send_task():
PROTOCOLS[protocol].check_supported(obj)
allow_opt_out = (obj.type == 'delete')

if (target not in obj.undelivered and target not in obj.failed
and 'force' not in request.values):
logger.info(f"{url} not in {obj.key.id()} undelivered or failed, giving up")
return r'¯\_(ツ)_/¯', 204

user = None
if user_key := form.get('user'):
key = ndb.Key(urlsafe=user_key)
Expand All @@ -1780,31 +1766,4 @@ def send_task():
if sent is False:
logger.info(f'Failed sending!')

# write results to Object
#
# retry aggressively because this has high contention during inbox delivery.
# (ndb does exponential backoff.)
# https://console.cloud.google.com/errors/detail/CJm_4sDv9O-iKg;time=P7D?project=bridgy-federated
@ndb.transactional(retries=10)
def update_object(obj_key):
obj = obj_key.get()
if target in obj.undelivered:
obj.remove('undelivered', target)

if sent is None:
obj.add('failed', target)
else:
if target in obj.failed:
obj.remove('failed', target)
if sent:
obj.add('delivered', target)

if not obj.undelivered:
obj.status = ('complete' if obj.delivered
else 'failed' if obj.failed
else 'ignored')
obj.put()

update_object(obj.key)

return '', 200 if sent else 204 if sent is False else 304
3 changes: 0 additions & 3 deletions scripts/opt_out.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,6 @@ def delete_ap_targets(*, from_proto=None, user=None, user_id=None):
targets += AP_BASE_TARGETS
targets = [Target(protocol='activitypub', uri=t) for t in targets]

obj.undelivered = targets
obj.put()

for target in targets:
assert util.is_web(target.uri), f'Non-URL target: {target.uri}'
params = {
Expand Down
23 changes: 0 additions & 23 deletions templates/activities.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,6 @@
<div class="col-xs-2">
{{ logs.maybe_link(obj.created, obj.key.id(), path=['/queue/webmention','/inbox'])|safe }}
</div>

<div class="col-xs-2">
<ul class="deliveries">
{% if obj.delivered %}
<li title="Delivered sucessfully">
<span class="glyphicon glyphicon-ok-sign"></span>
{{ obj.delivered|length }}
</li>
{% endif %}
{% if obj.undelivered %}
<li title="Remaining to be delivered">
<span class="glyphicon glyphicon-transfer"></span>
{{ obj.undelivered|length }}
</li>
{% endif %}
{% if obj.failed %}
<li title="Failed delivery">
<span class="glyphicon glyphicon-exclamation-sign"></span>
{{ obj.failed|length }}
</li>
{% endif %}
<ul>
</div>
</li>
{% else %}
<span class="big">No activity yet. Check back soon!</span>
Expand Down
31 changes: 1 addition & 30 deletions tests/test_activitypub.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -627,8 +627,7 @@ def test_inbox_transient_activity_generates_id(self, mock_head, mock_get,

expected_id = 'https://mas.to/users/swentel#Create-http://mas.to/note/id-2022-01-02T03:04:05+00:00'
self.assert_object(expected_id, source_protocol='activitypub',
as2=note, status='ignored', users=[user.key],
ignore=['our_as1'])
as2=note, users=[user.key], ignore=['our_as1'])

def test_inbox_bad_id(self, *_):
user = self.make_user(ACTOR['id'], cls=ActivityPub, obj_as2=ACTOR)
Expand Down Expand Up @@ -705,8 +704,6 @@ def test_inbox_reply_object(self, mock_head, mock_get, mock_post):
'http://mas.to/reply/id#bridgy-fed-create',
source_protocol='activitypub',
our_as1=expected_as1,
status='complete',
delivered=['https://user.com/post'],
type='post',
notify=[self.user.key],
users=[self.swentel_key],
Expand All @@ -733,8 +730,6 @@ def test_inbox_reply_object_wrapped(self, mock_head, mock_get, mock_post):
'http://mas.to/reply/id#bridgy-fed-create',
source_protocol='activitypub',
our_as1=expected_as1,
status='complete',
delivered=['https://user.com/post'],
type='post',
notify=[self.user.key],
users=[self.swentel_key],
Expand All @@ -756,8 +751,6 @@ def test_inbox_reply_create_activity(self, mock_head, mock_get, mock_post):
'http://mas.to/reply/as2',
source_protocol='activitypub',
as2=create,
status='complete',
delivered=['https://user.com/post'],
type='post',
notify=[self.user.key],
users=[self.swentel_key],
Expand Down Expand Up @@ -906,8 +899,6 @@ def _test_inbox_create_obj(self, path, mock_head, mock_get, mock_post):
our_as1=expected_create,
users=[ndb.Key(ActivityPub, ACTOR['id'])],
type='post',
status='complete',
delivered=['fake:shared:target'],
delivered_protocol='fake')

def test_repost_of_indieweb(self, mock_head, mock_get, mock_post):
Expand Down Expand Up @@ -943,14 +934,12 @@ def test_repost_of_indieweb(self, mock_head, mock_get, mock_post):

self.assert_object(REPOST_FULL['id'],
source_protocol='activitypub',
status='complete',
as2={
**REPOST,
'actor': ACTOR,
'object': repost['object'],
},
users=[self.swentel_key],
delivered=['https://user.com/orig'],
type='share',
)
def test_shared_inbox_repost_of_fediverse(self, mock_head, mock_get, mock_post):
Expand All @@ -977,12 +966,10 @@ def test_shared_inbox_repost_of_fediverse(self, mock_head, mock_get, mock_post):
uri='fake:o:ap:https://mas.to/users/alice/statuses/654/activity')
self.assert_object(REPOST['id'],
source_protocol='activitypub',
status='complete',
as2=REPOST,
copies=[copy],
users=[self.swentel_key],
feed=[baz.key, self.user.key],
delivered=['fake:shared:target'],
delivered_protocol='fake',
type='share',
ignore=['our_as1'])
Expand All @@ -1008,7 +995,6 @@ def test_inbox_no_user(self, mock_head, mock_get, mock_post):
self.assert_object('http://mas.to/like#ok',
# no nope.com Web user key since it didn't exist
source_protocol='activitypub',
status='ignored',
our_as1=as2.to_as1({
**LIKE_WITH_ACTOR,
'object': 'http://nope.com/post',
Expand Down Expand Up @@ -1133,9 +1119,7 @@ def test_inbox_like(self, mock_head, mock_get, mock_post):
notify=[self.user.key],
users=[self.masto_actor_key],
source_protocol='activitypub',
status='complete',
our_as1=as2.to_as1(LIKE_WITH_ACTOR),
delivered=['https://user.com/post'],
type='like',
)

Expand Down Expand Up @@ -1171,9 +1155,7 @@ def test_inbox_follow_accept_with_id(self, *mocks):
users=[self.swentel_key],
notify=[self.user.key],
source_protocol='activitypub',
status='complete',
our_as1=as2.to_as1(follow),
delivered=['https://user.com/'],
type='follow',
)

Expand All @@ -1198,9 +1180,7 @@ def test_inbox_follow_accept_with_object(self, *mocks):
users=[self.swentel_key],
notify=[self.user.key],
source_protocol='activitypub',
status='complete',
our_as1=as2.to_as1(follow),
delivered=['https://user.com/'],
type='follow',
)

Expand Down Expand Up @@ -1245,9 +1225,7 @@ def test_inbox_follow_accept_shared_inbox(self, mock_head, mock_get, mock_post):
users=[self.swentel_key],
notify=[self.user.key],
source_protocol='activitypub',
status='complete',
our_as1=as2.to_as1(follow),
delivered=['https://user.com/'],
type='follow',
)

Expand All @@ -1270,10 +1248,7 @@ def test_inbox_follow_accept_webmention_fails(self, mock_head, mock_get,
users=[self.swentel_key],
notify=[self.user.key],
source_protocol='activitypub',
status='failed',
our_as1=as2.to_as1(follow),
delivered=[],
failed=['https://user.com/'],
type='follow',
)

Expand Down Expand Up @@ -1521,7 +1496,6 @@ def test_inbox_bad_object_url(self, mock_head, mock_get, mock_post):
our_as1=expected,
users=[self.swentel_key],
source_protocol='activitypub',
status='ignored',
)
self.assertIsNone(Object.get_by_id(bad_url))

Expand Down Expand Up @@ -1735,7 +1709,6 @@ def test_delete_note(self, _, mock_get, ___):
as2=delete,
type='delete',
source_protocol='activitypub',
status='ignored',
users=[ActivityPub(id='https://mas.to/users/swentel').key])

def test_update_note(self, *mocks):
Expand Down Expand Up @@ -1771,7 +1744,6 @@ def _test_update(self, _, mock_get, ___):
self.assert_object(UPDATE_NOTE['id'],
source_protocol='activitypub',
type='update',
status='ignored',
our_as1=update_as1,
users=[self.swentel_key])

Expand Down Expand Up @@ -1810,7 +1782,6 @@ def test_inbox_no_webmention_endpoint(self, mock_head, mock_get, mock_post):
notify=[self.user.key],
users=[self.masto_actor_key],
source_protocol='activitypub',
status='ignored',
our_as1=as2.to_as1(LIKE_WITH_ACTOR),
type='like',
)
Expand Down
Loading

0 comments on commit 63c0e25

Please sign in to comment.