Send the moderator DM after responding, not before
docker-build-push / build-push (push) Successful in 5s
docker-build-push / build-push (push) Successful in 5s
The first real pre-block in production exposed this. Timeline from the logs: 00:24:54.53 Mastodon calls (signup form render) 00:24:55.33 ipapi.is answers (cold lookup, new range) 00:24:55.89 ip_blocks POST 200 <- block installed, 1.35s in 00:24:56.47 statuses POST 200 <- moderator DM, 582ms 00:24:56.53 Mastodon gives up at its 2.0s budget The block landed comfortably inside the budget and did its job -- the range is blocked and no account was created from it. But the answer never got back, because a moderator DM is a whole extra Mastodon round trip sitting on the synchronous response path. Nothing the caller acts on depends on that DM. It now goes out as a FastAPI BackgroundTask, which runs after the response is sent, so a cold lookup answers in ~1.35s instead of ~1.95s. Only classification and registering the block have to happen before we answer; telling a human can wait. reputation_check() takes a notify callable, defaulting to sending immediately so any non-HTTP caller keeps the old behaviour; the endpoint passes one that collects and defers. Tests assert the block is registered before the DM goes out, and that a DM that raises cannot stop a block being installed.
This commit is contained in:
1 parent
37feb109b3
commit
9e4456b7df
2 files changed
+66
-8
No files matched your search
+32
-8
@@ -1537,7 +1537,7 @@ def _external_lookup_allowed() -> bool:
|
|||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
||||||
def _reputation_ip(ip: str, source: str) -> dict | None:
|
def _reputation_ip(ip: str, source: str, notify) -> dict | None:
|
||||||
"""Classify one IP for the query API and install the block if it's flagged.
|
"""Classify one IP for the query API and install the block if it's flagged.
|
||||||
|
|
||||||
Returns None when the IP side is switched off or the address doesn't parse.
|
Returns None when the IP side is switched off or the address doesn't parse.
|
||||||
@@ -1587,7 +1587,7 @@ def _reputation_ip(ip: str, source: str) -> dict | None:
|
|||||||
log.warning("reputation query: pre-blocked ip=%s cidr=%s classification=%s org=%s source=%s",
|
log.warning("reputation query: pre-blocked ip=%s cidr=%s classification=%s org=%s source=%s",
|
||||||
ip, cidr, classification, org, source or "-")
|
ip, cidr, classification, org, source or "-")
|
||||||
if REPUTATION_API_DM_MODERATOR:
|
if REPUTATION_API_DM_MODERATOR:
|
||||||
dm_moderator(
|
notify(
|
||||||
f"🛡️ Pre-blocked {cidr} before signup — {classification} "
|
f"🛡️ Pre-blocked {cidr} before signup — {classification} "
|
||||||
f"({org or 'unknown org'})."
|
f"({org or 'unknown org'})."
|
||||||
)
|
)
|
||||||
@@ -1599,7 +1599,7 @@ def _reputation_ip(ip: str, source: str) -> dict | None:
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
def _reputation_email(email: str, source: str) -> dict | None:
|
def _reputation_email(email: str, source: str, notify) -> dict | None:
|
||||||
"""Classify one signup email's *domain* for the query API and install the
|
"""Classify one signup email's *domain* for the query API and install the
|
||||||
domain block if it's flagged. The full address is never stored or logged —
|
domain block if it's flagged. The full address is never stored or logged —
|
||||||
same rule as the webhook path (see classify_email_domain).
|
same rule as the webhook path (see classify_email_domain).
|
||||||
@@ -1640,7 +1640,7 @@ def _reputation_email(email: str, source: str) -> dict | None:
|
|||||||
log.warning("reputation query: pre-blocked domain=%s disposable=%s risk=%d source=%s",
|
log.warning("reputation query: pre-blocked domain=%s disposable=%s risk=%d source=%s",
|
||||||
domain, is_disposable, risk, source or "-")
|
domain, is_disposable, risk, source or "-")
|
||||||
if REPUTATION_API_DM_MODERATOR:
|
if REPUTATION_API_DM_MODERATOR:
|
||||||
dm_moderator(
|
notify(
|
||||||
f"🛡️ Pre-blocked email domain {domain} before signup "
|
f"🛡️ Pre-blocked email domain {domain} before signup "
|
||||||
f"(disposable={is_disposable}, risk={risk})."
|
f"(disposable={is_disposable}, risk={risk})."
|
||||||
)
|
)
|
||||||
@@ -1654,7 +1654,8 @@ def _reputation_email(email: str, source: str) -> dict | None:
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
def reputation_check(ip: str = "", email: str = "", source: str = "") -> dict:
|
def reputation_check(ip: str = "", email: str = "", source: str = "",
|
||||||
|
notify=None) -> dict:
|
||||||
"""Classify an IP and/or email domain, install any block they earn, and
|
"""Classify an IP and/or email domain, install any block they earn, and
|
||||||
report what happened. The write half of the bot's pre-emptive path.
|
report what happened. The write half of the bot's pre-emptive path.
|
||||||
|
|
||||||
@@ -1668,10 +1669,19 @@ def reputation_check(ip: str = "", email: str = "", source: str = "") -> dict:
|
|||||||
as a result of this call", which is what the caller acts on: it is true
|
as a result of this call", which is what the caller acts on: it is true
|
||||||
both for a block installed just now and for one we installed earlier that
|
both for a block installed just now and for one we installed earlier that
|
||||||
is still in force.
|
is still in force.
|
||||||
|
|
||||||
|
`notify` receives the moderator-alert text for each newly-installed block.
|
||||||
|
It defaults to sending immediately, but the HTTP endpoint passes one that
|
||||||
|
defers until after the response — a moderator DM is a whole extra Mastodon
|
||||||
|
round trip (measured at 582ms), and the caller is a live signup waiting on
|
||||||
|
a timeout. Only the classification and the block registration have to
|
||||||
|
happen before we answer; telling a human can wait.
|
||||||
"""
|
"""
|
||||||
|
if notify is None:
|
||||||
|
notify = dm_moderator
|
||||||
result = {
|
result = {
|
||||||
"ip": _reputation_ip(ip.strip(), source),
|
"ip": _reputation_ip(ip.strip(), source, notify),
|
||||||
"email": _reputation_email(email.strip(), source),
|
"email": _reputation_email(email.strip(), source, notify),
|
||||||
"checked_at": _now_iso(),
|
"checked_at": _now_iso(),
|
||||||
}
|
}
|
||||||
sides = [side for side in (result["ip"], result["email"]) if side]
|
sides = [side for side in (result["ip"], result["email"]) if side]
|
||||||
@@ -2031,6 +2041,7 @@ def _reputation_guard(key: str | None) -> None:
|
|||||||
@app.post("/api/reputation")
|
@app.post("/api/reputation")
|
||||||
def api_reputation(
|
def api_reputation(
|
||||||
body: ReputationRequest,
|
body: ReputationRequest,
|
||||||
|
background: BackgroundTasks,
|
||||||
x_reputation_key: str | None = Header(default=None),
|
x_reputation_key: str | None = Header(default=None),
|
||||||
):
|
):
|
||||||
"""Look up an IP and/or email domain, record the answer, install whatever
|
"""Look up an IP and/or email domain, record the answer, install whatever
|
||||||
@@ -2040,11 +2051,24 @@ def api_reputation(
|
|||||||
Declared as a plain `def` on purpose: FastAPI runs those in a threadpool,
|
Declared as a plain `def` on purpose: FastAPI runs those in a threadpool,
|
||||||
so the blocking provider calls underneath never tie up the event loop and
|
so the blocking provider calls underneath never tie up the event loop and
|
||||||
stall webhook deliveries.
|
stall webhook deliveries.
|
||||||
|
|
||||||
|
Moderator DMs are deferred to after the response. The caller is a live
|
||||||
|
signup on a ~2s budget, and a cold lookup already spends most of it on the
|
||||||
|
provider call plus registering the block; adding a Mastodon status POST on
|
||||||
|
top blew the budget in production the first time a real bad range showed
|
||||||
|
up (the block landed at 1.35s, the DM pushed the answer past 2.0s and the
|
||||||
|
caller timed out). Nothing the caller acts on depends on the DM.
|
||||||
"""
|
"""
|
||||||
_reputation_guard(x_reputation_key)
|
_reputation_guard(x_reputation_key)
|
||||||
if not body.ip and not body.email:
|
if not body.ip and not body.email:
|
||||||
raise HTTPException(status_code=400, detail="ip or email is required")
|
raise HTTPException(status_code=400, detail="ip or email is required")
|
||||||
return reputation_check(ip=body.ip, email=body.email, source=body.source)
|
|
||||||
|
alerts: list[str] = []
|
||||||
|
result = reputation_check(ip=body.ip, email=body.email, source=body.source,
|
||||||
|
notify=alerts.append)
|
||||||
|
for message in alerts:
|
||||||
|
background.add_task(dm_moderator, message)
|
||||||
|
return result
|
||||||
|
|
||||||
|
|
||||||
@app.post("/webhook")
|
@app.post("/webhook")
|
||||||
|
|||||||
@@ -1036,6 +1036,8 @@ def reputation_api_tests():
|
|||||||
main.register_ip_block = (
|
main.register_ip_block = (
|
||||||
lambda ip, acct, org, cidr="", comment="":
|
lambda ip, acct, org, cidr="", comment="":
|
||||||
ipblocks.append((ip, cidr, comment)) or True)
|
ipblocks.append((ip, cidr, comment)) or True)
|
||||||
|
# TestClient runs BackgroundTasks to completion before returning, so the
|
||||||
|
# deferred moderator DMs are still observable in `dms` after a request.
|
||||||
main.register_email_domain_block = (
|
main.register_email_domain_block = (
|
||||||
lambda domain, acct: domain_blocks.append((domain, acct)) or True)
|
lambda domain, acct: domain_blocks.append((domain, acct)) or True)
|
||||||
|
|
||||||
@@ -1251,6 +1253,38 @@ def reputation_api_tests():
|
|||||||
active = {e["cidr"] for e in client.get("/api/blocks/ip-ranges").json()["items"]}
|
active = {e["cidr"] for e in client.get("/api/blocks/ip-ranges").json()["items"]}
|
||||||
assert "203.0.113.0/24" not in active, "an expired block must not be published"
|
assert "203.0.113.0/24" not in active, "an expired block must not be published"
|
||||||
|
|
||||||
|
# N. The moderator DM is deferred until after the response. It costs a
|
||||||
|
# whole Mastodon round trip (582ms measured), and the caller is a live
|
||||||
|
# signup on a ~2s budget -- keeping it inline blew that budget the
|
||||||
|
# first time a real bad range showed up in production.
|
||||||
|
sent_order = []
|
||||||
|
main.register_ip_block = (
|
||||||
|
lambda ip, acct, org, cidr="", comment="":
|
||||||
|
sent_order.append("block") or ipblocks.append((ip, cidr, comment)) or True)
|
||||||
|
main.dm_moderator = lambda message: sent_order.append("dm") or dms.append(message)
|
||||||
|
classifications["198.51.103.7"] = ("datacenter", "Late DM Inc", True, "198.51.103.0/24")
|
||||||
|
|
||||||
|
dms.clear()
|
||||||
|
r = ask({"ip": "198.51.103.7", "source": "defer-check"})
|
||||||
|
assert r.json()["blocked"] is True, r.json()
|
||||||
|
assert sent_order == ["block", "dm"], \
|
||||||
|
f"the block must be registered before the DM is sent, got {sent_order}"
|
||||||
|
assert any("198.51.103.0/24" in d for d in dms), "the DM still goes out, just later"
|
||||||
|
|
||||||
|
# ...and the response body never depends on the DM having been sent.
|
||||||
|
main.dm_moderator = lambda message: (_ for _ in ()).throw(
|
||||||
|
RuntimeError("moderator DM must not be able to break a lookup"))
|
||||||
|
classifications["198.51.104.7"] = ("datacenter", "Broken DM Inc", True, "198.51.104.0/24")
|
||||||
|
try:
|
||||||
|
r = ask({"ip": "198.51.104.7", "source": "defer-check"})
|
||||||
|
except RuntimeError:
|
||||||
|
# TestClient surfaces background-task errors; the important part is
|
||||||
|
# that the block was still installed and the answer computed first.
|
||||||
|
pass
|
||||||
|
assert main.reputation_block_installed("ip", "198.51.104.0/24"), \
|
||||||
|
"a failing DM must not prevent the block being registered"
|
||||||
|
main.dm_moderator = lambda message: dms.append(message)
|
||||||
|
|
||||||
# Email domain blocks don't expire in Mastodon, so they carry none here
|
# Email domain blocks don't expire in Mastodon, so they carry none here
|
||||||
# either: recorded with no expiry, they stay installed indefinitely.
|
# either: recorded with no expiry, they stay installed indefinitely.
|
||||||
main.record_reputation_block("email", "rep-forever.test", "", "")
|
main.record_reputation_block("email", "rep-forever.test", "", "")
|
||||||
|
|||||||
Reference in new issue
Block a user