Skip to content

Commit 4697206

Browse files
committed
fix: restriction race conditions, permanent dead-state, event-loop lock bug
Critical fixes from 4-agent parallel review of feat/guest-bot-block: - guest_bot.py: revert threshold check to < (not !=) and drop the increment-on-failure. != combined with the increment permanently wedged a caller into delete-only mode after one failed restrict_chat_member call. < without incrementing on failure pins the count at threshold, so the next guest message retries restriction instead of drifting past it forever or looping tightly. - captcha_recovery.py: serialize the pending-captcha read + remove inside restriction_lock and check remove_pending_captcha's return value. The timeout path previously read/removed outside the lock, racing the captcha callback and leaving is_restricted=True in DB for an already unrestricted-on-Telegram user. - message.py, scheduler.py: in-lock recheck now verifies the fresh warning row is the SAME row that reached the threshold/expired (fresh.id != record.id), not just that some active row exists. Fixes restricting a brand-new low-count row on the strength of a stale one. - restriction_lock.py: scope the lock cache per running event loop via a WeakKeyDictionary instead of a flat process-global dict. asyncio.Lock binds to the event loop of its first contended acquire; reusing a key across event loops (e.g. per-test loops) previously raised RuntimeError. - verify.py: widen the unrestrict except clause to TelegramError (was a narrow tuple missing RetryAfter/ChatMigrated), matching the bare Exception catch already used by the sibling unrestrict_user_in_group. - guest_bot.py: drop the outer except TelegramError around the lock body — nothing inside can raise TelegramError past the inner handler, so it was dead code that obscured control flow. - tests/test_scheduler.py: fix a test mock that always returned the same warning row regardless of user_id, which the new row-identity check correctly rejected for the second user. - tests/test_restriction_lock.py: update for the loop-scoped lock storage. - AGENTS.md, README.md: correct guest_bot_block handler_group (1 -> 0), stale Code Map line counts, test count (1,064 -> 1,075), mermaid guest gate label and missing whitelisted-passthrough edge, and the dispatch rationale (no GROUPS & ~COMMAND handler exists at group 0). All 1075 tests passing, ruff/mypy clean.
1 parent 110b450 commit 4697206

10 files changed

Lines changed: 99 additions & 77 deletions

File tree

AGENTS.md

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ PythonID/
8181
│ │ └── admin_cache.py # Admin ID cache + refresh
8282
│ └── database/
8383
│ ├── models.py # SQLModel schemas (5 tables: UserWarning, PhotoVerificationWhitelist, PendingCaptchaValidation, NewUserProbation, TrustedUser)
84-
│ └── service.py # DatabaseService singleton (645 lines)
84+
│ └── service.py # DatabaseService singleton (958 lines)
8585
├── tests/ # pytest-asyncio + Hypothesis (30+ files)
8686
│ ├── test_properties.py # Property-based tests for pure functions
8787
│ └── test_warn.py # /warn command tests (23 tests)
@@ -102,33 +102,33 @@ PythonID/
102102
| Add Telegram whitelist | `constants.py``WHITELISTED_TELEGRAM_PATHS` | Lowercase, exact path match |
103103
| Multi-group config | `group_config.py` | GroupConfig model, GroupRegistry, groups.json loading |
104104
| Warn a member | `handlers/warn.py` + `plugins/builtin/commands.py` | Admin `/warn` by reply or user ID; registered as `warn_command` |
105-
| Block guest bots | `handlers/guest_bot.py` + `plugins/builtin/spam.py` | `guest_bot_block` plugin (group=1); whitelist via `GUEST_BOT_WHITELIST` env or `guest_bot_whitelist` per-group JSON |
105+
| Block guest bots | `handlers/guest_bot.py` + `plugins/builtin/spam.py` | `guest_bot_block` plugin (group=0); whitelist via `GUEST_BOT_WHITELIST` env or `guest_bot_whitelist` per-group JSON |
106106

107107
## Code Map (Key Files)
108108

109109
| File | Lines | Role |
110110
|------|-------|------|
111-
| `database/service.py` | 850 | **Complexity hotspot** - handles warnings, captcha, probation state |
111+
| `database/service.py` | 958 | **Complexity hotspot** - handles warnings, captcha, probation state |
112112
| `constants.py` | 724 | Templates + massive whitelists (Indonesian tech community) |
113113
| `handlers/anti_spam.py` | 494 | Anti-spam: contact cards, inline keyboards, probation enforcement |
114114
| `handlers/bio_bait.py` | 441 | Bio-bait spam: obfuscated bait phrases + suspicious profile bio links |
115-
| `handlers/guest_bot.py` | 118 | Guest Mode moderation: delete non-whitelisted guest bot messages + progressive restriction of the human caller |
115+
| `handlers/guest_bot.py` | 133 | Guest Mode moderation: delete non-whitelisted guest bot messages + progressive restriction of the human caller |
116116
| `handlers/check.py` | 437 | Admin /check: group selector + group-scoped action buttons |
117117
| `handlers/captcha.py` | 427 | New member join → restrict → verify (with profile check) → unrestrict lifecycle |
118-
| `handlers/verify.py` | 400 | Photo exemption + bot-owned unrestriction (group-scoped) |
118+
| `handlers/verify.py` | 422 | Photo exemption + bot-owned unrestriction (group-scoped) |
119119
| `handlers/trust.py` | 368 | /trust, /untrust, /trusted admin commands (no auto-unrestrict) |
120-
| `handlers/dm.py` | 250 | DM unrestriction flow with deep-link group recovery |
121-
| `handlers/message.py` | 208 | Profile compliance monitoring + stale warning clearing |
120+
| `handlers/dm.py` | 264 | DM unrestriction flow with deep-link group recovery |
121+
| `handlers/message.py` | 217 | Profile compliance monitoring + stale warning clearing |
122122
| `handlers/status.py` | 181 | Group-scoped /status (admin's groups only, Indonesian labels) |
123123
| `handlers/warn.py` | 170 | Admin-issued generic warning by reply or user ID; optional moderation-topic routing |
124-
| `services/scheduler.py` | 151 | Auto-restriction with pre-restriction profile recheck |
124+
| `services/scheduler.py` | 160 | Auto-restriction with pre-restriction profile recheck |
125125
| `group_config.py` | 255 | Multi-group config, registry, JSON loading, .env fallback |
126126
| `main.py` | 191 | Entry point, logging, post_init, PluginManager bootstrap |
127127
| `plugins/manager.py` | 188 | PluginManager — static registry + deterministic registration order |
128128
| `plugins/config.py` | 156 | `guard_plugin` runtime gate + toggle resolution |
129129
| `plugins/definitions.py` | 72 | `MANIFEST_ORDER` / `PLUGIN_NAMES` — single source of truth for plugin names + groups |
130130
| `plugins/builtin/commands.py` | 166 | Wraps all command + callback handlers with group-scoped patterns |
131-
| `plugins/builtin/spam.py` | 93 | Wraps all 5 anti-spam handlers + guest_bot_block with `guard_plugin` |
131+
| `plugins/builtin/spam.py` | 114 | Wraps all 5 anti-spam handlers + guest_bot_block with `guard_plugin` |
132132
| `plugins/builtin/captcha.py` | 43 | Wraps captcha handler + applies guard_plugin gating |
133133

134134
## Architecture Patterns
@@ -148,8 +148,8 @@ PythonID/
148148
```python
149149
# Registration order comes from MANIFEST_ORDER (plugins/definitions.py), not main.py directly
150150
group=-1 # topic_guard: Runs FIRST
151-
group=0 # commands (including warn_command), callbacks, captcha, dm (18 plugins, order-independent)
152-
group=1 # inline_keyboard_spam + guest_bot_block: Catches inline keyboard URL spam / Guest Mode messages
151+
group=0 # commands (including warn_command), callbacks, captcha, dm, guest_bot_block (19 plugins; only guest_bot_block is order-sensitive, via ApplicationHandlerStop)
152+
group=1 # inline_keyboard_spam: Catches inline keyboard URL spam
153153
group=2 # contact_spam: Blocks contact card sharing
154154
group=3 # new_user_spam: Probation enforcement (links/forwards)
155155
group=4 # duplicate_spam + bio_bait_spam: Repeated messages / bio-bait detection
@@ -179,12 +179,13 @@ group=6 # JobQueue only (not a handler group): auto_restrict_job, refresh_admi
179179

180180
### Guest Bot Moderation
181181
- `handlers/guest_bot.py` blocks Telegram **Guest Mode** messages — messages posted by a bot on behalf of a user/channel via the `@` mention feature that Telegram routes through `message.guest_bot_caller_user` / `message.guest_bot_caller_chat` (PTB v22.8+)
182-
- A custom `GuestBotFilter` (`filters.MessageFilter`) matches only messages where either guest-bot caller field is set, so the handler is dispatched before the broad `GROUPS & ~COMMAND` filters at the same group
183-
- Registered as `guest_bot_block` at `handler_group=1` via `spam_mod.register_guest_bot_block` in `plugins/builtin/spam.py`, gated by `guard_plugin("guest_bot_block")`
182+
- A custom `GuestBotFilter` (`filters.MessageFilter`) matches only messages where either guest-bot caller field is set, so the handler only fires on actual Guest Mode updates and raises `ApplicationHandlerStop` to stop the spam handlers at groups 1-5 from also processing the message
183+
- Registered as `guest_bot_block` at `handler_group=0` (alongside commands/callbacks/captcha/dm) via `spam_mod.register_guest_bot_block` in `plugins/builtin/spam.py`, gated by `guard_plugin("guest_bot_block")`
184184
- Non-whitelisted guest bot messages are always deleted; the invoking **human caller** then receives progressive enforcement using the group's existing `warning_threshold`:
185185
- 1st violation → warning in the warning topic (`GUEST_BOT_WARNING`)
186186
- 2nd to (N-1) → silent increment
187187
- Nth violation → restrict + notification (`GUEST_BOT_RESTRICTION`)
188+
- If `restrict_chat_member` fails (e.g. bot lacks ban rights), the strike count is NOT incremented past the threshold — the next guest message from the same caller retries the restriction instead of drifting into a permanent delete-only state
188189
- Admin/trusted callers have their guest message deleted but are **not** warned or restricted
189190
- Chat/channel-only callers (no `guest_bot_caller_user`) are delete-only — there is no human to warn
190191
- Already guest-bot-restricted callers do not start a fresh warning cycle (`is_user_restricted_by_bot` check)
@@ -331,7 +332,7 @@ if user.id not in admin_ids:
331332
- **Trust feature**: `TrustedUser` table caches user_full_name + admin_full_name at trust time so `/trusted` lists admin info without Telegram API calls. Backfill script at `scripts/backfill_trusted_names.py` for pre-existing rows
332333
- **Local review artifacts**: `reviews/` directory contains output from parallel reviewer subagents. Gitignored; not part of the source tree
333334
- **Captcha DB ordering**: The captcha callback handler calls Telegram `unrestrict_user` BEFORE DB writes (remove_pending_captcha, start_new_user_probation). If unrestrict fails, the pending captcha stays in DB and the user can retry. DB finalization is idempotent — `remove_pending_captcha` returning False means a concurrent callback already finalized
334-
- **Guest bot blocking**: Telegram Guest Mode lets any user `@mention` a bot and have the result posted in a chat. The `guest_bot_block` plugin (group=1) deletes non-whitelisted guest bot messages and progressively restricts the human caller (1st=warning, Nth=restrict). Admins/trusted users and channel-only callers are delete-only. Bot whitelist is case-insensitive with optional `@`. Guest strikes are tracked separately via `warning_kind="guest_bot"` and are not eligible for the DM self-service unrestriction flow
335+
- **Guest bot blocking**: Telegram Guest Mode lets any user `@mention` a bot and have the result posted in a chat. The `guest_bot_block` plugin (group=0) deletes non-whitelisted guest bot messages and progressively restricts the human caller (1st=warning, Nth=restrict). Admins/trusted users and channel-only callers are delete-only. Bot whitelist is case-insensitive with optional `@`. A failed restriction does not increment the strike count past the threshold, so the caller's next guest message retries the restriction. Guest strikes are tracked separately via `warning_kind="guest_bot"` and are not eligible for the DM self-service unrestriction flow
335336

336337
## Policy
337338

README.md

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ uv run mypy src/bot/ tests/
212212

213213
The project maintains comprehensive test coverage:
214214
- **Coverage**: 97%+ (~2,900 statements, <3% unreachable)
215-
- **Tests**: 1,064 total (includes 19 Hypothesis property tests)
215+
- **Tests**: 1,075 total (includes 19 Hypothesis property tests)
216216
- **Pass Rate**: 100%
217217
- **Property tests**: `tests/test_properties.py` exercises pure functions (format helpers, URL whitelist, name formatters) with random inputs and shrinks failing cases to minimal examples
218218
- **Mypy**: Pragmatic config in `pyproject.toml`. Disables error codes that are noisy from PTB / SQLModel / Pydantic v2; catches real type bugs in new code
@@ -369,8 +369,9 @@ flowchart TD
369369
G_TopicIsBotAdmin -->|No| G_TopicDelete[Delete Message]
370370
G_TopicDelete --> StopTopic2([ApplicationHandlerStop])
371371
372-
G_GuestGate{group=1 guest_bot_block:<br/>Guest Mode message?}
372+
G_GuestGate{group=0 guest_bot_block:<br/>Guest Mode message?}
373373
G_GuestGate -->|No| G_InlineGate
374+
G_GuestGate -->|Yes, whitelisted| G_InlineGate
374375
G_GuestGate -->|Yes, not whitelisted| G_GuestDelete[Delete Message]
375376
G_GuestDelete --> G_GuestCaller{Human caller?<br/>Admin/Trusted?}
376377
G_GuestCaller -->|Admin/Trusted or Channel-only| StopGuest([ApplicationHandlerStop])

src/bot/handlers/guest_bot.py

Lines changed: 33 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -75,46 +75,43 @@ async def handle_guest_bot_message(update: Update, context: ContextTypes.DEFAULT
7575
if record.message_count >= group_config.warning_threshold:
7676
should_stop = False
7777
final_count = record.message_count
78-
try:
79-
async with restriction_lock(group_config.group_id, caller.id):
80-
if db.is_user_restricted_by_bot(caller.id, group_config.group_id, warning_kind="guest_bot"):
78+
async with restriction_lock(group_config.group_id, caller.id):
79+
if db.is_user_restricted_by_bot(caller.id, group_config.group_id, warning_kind="guest_bot"):
80+
should_stop = True
81+
else:
82+
fresh = db.get_or_create_user_warning(caller.id, group_config.group_id, warning_kind="guest_bot")
83+
if fresh.message_count < group_config.warning_threshold:
8184
should_stop = True
8285
else:
83-
fresh = db.get_or_create_user_warning(caller.id, group_config.group_id, warning_kind="guest_bot")
84-
if fresh.message_count != group_config.warning_threshold:
86+
try:
87+
await context.bot.restrict_chat_member(
88+
chat_id=group_config.group_id,
89+
user_id=caller.id,
90+
permissions=RESTRICTED_PERMISSIONS,
91+
)
92+
db.mark_user_restricted(caller.id, group_config.group_id, warning_kind="guest_bot")
93+
final_count = fresh.message_count
94+
except TelegramError as e:
95+
logger.error("Failed to restrict guest bot caller %s: %s", caller.id, e, exc_info=True)
96+
# Do not increment on failure: count stays pinned at
97+
# threshold so the next guest message retries the
98+
# restriction instead of drifting past it forever.
8599
should_stop = True
86-
else:
87-
try:
88-
await context.bot.restrict_chat_member(
89-
chat_id=group_config.group_id,
90-
user_id=caller.id,
91-
permissions=RESTRICTED_PERMISSIONS,
92-
)
93-
db.mark_user_restricted(caller.id, group_config.group_id, warning_kind="guest_bot")
94-
final_count = fresh.message_count
95-
except TelegramError as e:
96-
logger.error("Failed to restrict guest bot caller %s: %s", caller.id, e, exc_info=True)
97-
should_stop = True
98-
db.increment_message_count(caller.id, group_config.group_id, warning_kind="guest_bot")
100+
if should_stop:
101+
raise ApplicationHandlerStop
102+
try:
103+
await context.bot.send_message(
104+
chat_id=group_config.group_id,
105+
message_thread_id=group_config.warning_topic_id,
106+
text=GUEST_BOT_RESTRICTION.format(
107+
user_mention=user_mention,
108+
message_count=final_count,
109+
rules_link=group_config.rules_link,
110+
),
111+
parse_mode="Markdown",
112+
)
99113
except TelegramError:
100-
logger.error("Failed to restrict guest bot caller %s (lock level)", caller.id, exc_info=True)
101-
should_stop = True
102-
else:
103-
if should_stop:
104-
raise ApplicationHandlerStop
105-
try:
106-
await context.bot.send_message(
107-
chat_id=group_config.group_id,
108-
message_thread_id=group_config.warning_topic_id,
109-
text=GUEST_BOT_RESTRICTION.format(
110-
user_mention=user_mention,
111-
message_count=final_count,
112-
rules_link=group_config.rules_link,
113-
),
114-
parse_mode="Markdown",
115-
)
116-
except TelegramError:
117-
logger.error("Failed to send guest bot restriction notice for user %s", caller.id, exc_info=True)
114+
logger.error("Failed to send guest bot restriction notice for user %s", caller.id, exc_info=True)
118115
elif record.message_count == 1:
119116
try:
120117
await context.bot.send_message(

src/bot/handlers/message.py

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,11 @@ async def handle_message(update: Update, context: ContextTypes.DEFAULT_TYPE) ->
159159
try:
160160
async with restriction_lock(group_config.group_id, user.id):
161161
fresh = db.get_active_user_warning(user.id, group_config.group_id)
162-
if fresh is None or fresh.is_restricted:
162+
if (
163+
fresh is None
164+
or fresh.id != record.id
165+
or fresh.message_count < group_config.warning_threshold
166+
):
163167
logger.info(
164168
f"Skipping profile restriction for user {user.id} - "
165169
f"record no longer active (group_id={group_config.group_id})"
@@ -182,13 +186,13 @@ async def handle_message(update: Update, context: ContextTypes.DEFAULT_TYPE) ->
182186
# Send restriction notice with DM link for appeal
183187
restriction_message = RESTRICTION_MESSAGE_AFTER_MESSAGES.format(
184188
user_mention=user_mention,
185-
message_count=record.message_count,
189+
message_count=fresh.message_count,
186190
missing_text=missing_text,
187191
rules_link=group_config.rules_link,
188192
dm_link=dm_link,
189193
)
190194
logger.info(
191-
f"Sending restriction notice: user_id={user.id}, user={user.full_name}, message_count={record.message_count}"
195+
f"Sending restriction notice: user_id={user.id}, user={user.full_name}, message_count={fresh.message_count}"
192196
)
193197
await context.bot.send_message(
194198
chat_id=group_config.group_id,
@@ -197,7 +201,7 @@ async def handle_message(update: Update, context: ContextTypes.DEFAULT_TYPE) ->
197201
parse_mode="Markdown",
198202
)
199203
logger.info(
200-
f"Restricted user {user.id} ({user.full_name}) after {record.message_count} messages (group_id={group_config.group_id})"
204+
f"Restricted user {user.id} ({user.full_name}) after {fresh.message_count} messages (group_id={group_config.group_id})"
201205
)
202206
except Exception:
203207
logger.error(

src/bot/handlers/verify.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
import logging
1515

1616
from telegram import Bot, Update
17-
from telegram.error import BadRequest, Forbidden, NetworkError, TimedOut
17+
from telegram.error import TelegramError
1818
from telegram.ext import ContextTypes
1919

2020
from bot.constants import (
@@ -90,7 +90,7 @@ async def verify_user_in_group(
9090
logger.info(
9191
f"Unrestricted user {target_user_id} in group {group_id} during verification"
9292
)
93-
except (BadRequest, Forbidden, NetworkError, TimedOut, RuntimeError) as e:
93+
except (TelegramError, RuntimeError) as e:
9494
logger.info(
9595
f"Could not unrestrict user {target_user_id} in group {group_id}: {e}"
9696
)

src/bot/services/captcha_recovery.py

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -46,16 +46,20 @@ async def handle_captcha_expiration(
4646
user_full_name: The user's full name.
4747
"""
4848
db = get_database()
49-
pending = db.get_pending_captcha(user_id, group_id)
50-
if not pending:
51-
logger.info(f"No pending captcha for user {user_id}, already verified")
52-
return
53-
54-
db.remove_pending_captcha(user_id, group_id)
5549

56-
# Create UserWarning to track this bot-applied restriction
57-
# Allows DM handler to unrestrict user later when profile is complete
5850
async with restriction_lock(group_id, user_id):
51+
pending = db.get_pending_captcha(user_id, group_id)
52+
if not pending:
53+
logger.info(f"No pending captcha for user {user_id}, already verified")
54+
return
55+
56+
removed = db.remove_pending_captcha(user_id, group_id)
57+
if not removed:
58+
logger.info(f"Captcha for user {user_id} already finalized, ignoring timeout")
59+
return
60+
61+
# Create UserWarning to track this bot-applied restriction
62+
# Allows DM handler to unrestrict user later when profile is complete
5963
warning = db.get_or_create_user_warning(user_id, group_id)
6064
if not warning.is_restricted:
6165
db.mark_user_restricted(user_id, group_id)

0 commit comments

Comments
 (0)