Review finding (low, real): the comment claimed /ping degenerates to a pure
connectivity check when TG_WATCHDOG_ENABLED=false, but _wd_last_ok_monotonic is
also stamped by _restart_client (on the disconnect-flap path, which runs before
the watchdog-enabled gate), so with the watchdog off one flap sets age and nothing
ever refreshes it — age grows unbounded past the threshold and /ping returns 503 on
a live connection, spuriously failing the container healthcheck and triggering an
autoheal restart after every flap.
Fix: gate the staleness branch on the watchdog being enabled —
healthy = connected and (not Config["tg_watchdog_enabled"] or age is None or age < threshold)
so with the watchdog disabled /ping is a pure connectivity check (matching the
intent), and correct the comment to note a flap-restart can stamp age even when the
watchdog is off. New test test_ping_watchdog_disabled_stale_age_still_healthy:
watchdog off + connected + stale age => 200 ok. Adversarially validated — reverting
the gate reds the new test (503) while the watchdog-ON stale-probe test stays green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The container healthcheck hit /rss/...?limit=1 (5s timeout): on a cold cache RSS
generation exceeds 5s, or a hung TG RPC makes it hang, so docker/autoheal restarts
the container mid-download and corrupts temp files. Replace it with /ping, which
reflects process/loop liveness (answers instantly, always) plus TG liveness read
from the watchdog's last-probe data — issuing ZERO Telegram RPC.
- telegram_client: public watchdog_last_ok_age() — seconds since the last successful
watchdog probe (None if never). Pure read of the Stage-1 _wd_last_ok_monotonic
field; no RPC.
- api_server: /ping route (no token, no TG RPC, no SQLite, no fs scan). healthy =
connected and (age is None or age < threshold). age is None right after boot =>
healthy (don't kill before the first probe). connected coerced to bool so the JSON
"connected" field is always a bool (pre-start reports false, never null).
- config: TG_PING_UNHEALTHY_AFTER knob, default interval*(failures+1)+timeout = 250s
(how long until the watchdog itself gives up), env-overridable.
- dockercompose.yml: healthcheck -> curl -sf http://127.0.0.1:80/ping, interval 5m,
timeout 5s, retries 3, start_period 30s. Old /rss check removed, not left behind.
- tests: 10 (healthy/stale/disconnected/fresh-boot/pre-start-null/no-token +
the anti-regression zero-TG-RPC spy across all branches + the accessor).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review findings (both low, no bugs; accumulator design adversarially confirmed):
- test-coverage: the DoD's hottest changed site — get_media's pre-semaphore
cache-hit — had no direct zero-SQLite guard (the spy test only exercised
download_media_file), so a regression re-introducing a per-hit write into
the get_media branch would pass green. Add a mirror spy test through
get_media asserting the accumulator is written and update_media_file_access_sync
is NOT called. Verified site-specific: neutering only the get_media write
reds the new test while the download_media_file test stays green.
- documentation: the _access_updates comment claimed a str/int key mix "would
make the WHERE silently never match" — empirically false: channel is a TEXT
column, so a bound int is affinity-coerced and DOES match. Reword to say we
key str(channel) to stay consistent with the stored form rather than lean on
SQLite's implicit coercion (the code was already correct on both sites).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Stage 5. A /media cache hit no longer touches SQLite: it records the access
timestamp into a module-level accumulator (a dict write on the event loop,
cheap and atomic), and a supervised 60s background task flushes the whole
batch in one executemany UPDATE. This removes both per-hit write sites — the
awaited to_thread in download_media_file and the fire-and-forget create_task
in the pre-semaphore fast path — so under active RSS polling the threadpool is
no longer starved by per-request access-time UPDATEs.
- file_io: add update_media_file_access_bulk_sync (one connection, executemany;
empty batch is a no-op).
- api_server: _access_updates accumulator + _flush_access_updates (snapshot-
then-clear atomically before the await so writes during the flush land in the
fresh dict; re-queue the batch with setdefault on write failure so a fresher
concurrent write is never clobbered and no access-time is lost) +
_access_flush_loop under _supervised + a final flush on shutdown, ordered
after the loop task is cancelled and before the io threadpool is shut down.
- Keys use str(channel) to match the TEXT channel column (a str/int mix would
make the UPDATE WHERE silently never match, evicting still-used files).
- tests/test_stage5_sqlite.py: 7 tests (no-sync-write hot path, str-key
discipline, hit->flush->DB, empty no-op, snapshot-then-clear race, re-queue-
without-clobbering-fresh, bulk SQL).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#1 [security] The final feed-sanitize try/except was FAIL-OPEN: since 4.4 removed
the per-fragment passes, post['html'] / the concatenated feed html is now raw
channel-controlled HTML, and this is its ONLY sanitize. If bleach itself throws
(RecursionError/OOM on pathological nested HTML — a class already seen in this
project), the except branch returned the RAW payload = stored XSS. Both branches
(RSS per-post, HTML whole-feed) now fail CLOSED via html.escape, so the content
survives as inert text and no live tag ever reaches the client. Added a test that
forces bleach to raise and asserts no live <script>/<img>/<a> reaches the feed
(adversarially validated: reverting to fail-open makes it fail).
#2 [test] Direct test of the new upsert_media_file_ids_bulk_sync on a temp DB:
empty no-op, multi-row insert, and re-upsert-updates-added (the real executemany +
ON CONFLICT, previously only mocked).
#3 [test] Test that media ids collected before a render exception are still
flushed (the flush is in a finally) — removing the finally now turns a test red.
#4 [cleanup] Removed the dead in rss_generator (the
flush is delegated to post_parser._flush_pending_media_ids).
#5 [doc] Corrected the sanitize-map comments: RSS sanitizes per-post, only HTML is
the whole-feed pass.
233 passed (214 baseline + 19).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Feed generation blocked the loop (deepcopy of hundreds of Messages + per-message
bleach x4 + str(message) per post). This moves the CPU work off the loop and cuts
it down.
4.1 raw_message is lazy: process_message(include_raw=False) omits str(message)
entirely; only /json and debug-HTML compute it.
4.2 (done BEFORE 4.3) _save_media_file_ids no longer touches asyncio/DB — it
appends to the per-request PostParser._pending_media_ids, and the caller
flushes once via to_thread(upsert_media_file_ids_bulk_sync) (a new executemany
fn in file_io.py). Removed _persist_pending_count + the create_task machinery.
This had to precede 4.3 or get_running_loop() inside the render thread would
raise and silently kill media-id persistence.
4.3 The render pipeline (_create_time_based_media_groups, _create_messages_groups,
_trim_messages_groups, _render_messages_groups) is now plain-sync and runs in
ONE asyncio.to_thread(_render_pipeline) from both feed generators; deepcopy
moved into the thread. The render path is verified free of asyncio primitives.
4.4 Sanitize is now ONE pass per output boundary (removed the 4 internal
per-fragment bleach passes): RSS/HTML feeds sanitize once at the whole-feed
pass; /html and /json sanitize body+footer once in process_message; the debug
<pre> raw dump is html.escape'd. Media embeds in body, reactions in footer, so
both are covered by the boundary pass. XSS tests (<script>/onerror=/javascript:)
confirm all four outputs are clean.
Review round-1: documented that flags are now extracted from the pre-sanitize body
(a 4.4 consequence — non-security, legitimate links unaffected); added
media-caption XSS tests for the exact fragments whose internal passes were removed
(adversarially validated: neutering the sanitizer makes them fail); the media-id
flush is now in a finally so a partial render still persists what it collected.
230 passed (214 baseline + 16).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
#1 [medium] FileResponse with stat_result=None re-stats at send-time and raises a
RuntimeError (-> 500 + stacktrace, escaping get_media's try/except) if the file was
swept between the early exists() check and the send. The old streaming code caught
FileNotFoundError on getsize -> 404. Now prepare_file_response takes ONE
authoritative os.stat (in try/except FileNotFoundError -> 404) after the temp_*
touch and passes it as FileResponse(stat_result=...): restores the 404 semantics,
collapses the double stat into one, and makes the ETag reflect the observed mtime.
The remaining narrow deleted-between-our-stat-and-open window pre-existed.
#2/#3 [doc] Corrected the Content-Disposition comment (FileResponse emits
filename="x" for ASCII, filename*=UTF-8''x only for non-ASCII, via setdefault — a
manual header would override, not double) and the ETag-stability comment (stable
within a 300s window; a longer view costs at most one safe 200 If-Range restart
per interval).
#4 [simplification] Removed the now-dead StreamingResponse import and corrected
get_media's return annotation to -> Response (nothing returns StreamingResponse
after the FileResponse migration).
#5 [test] Added a test for the middleware's non-http (lifespan/websocket) branch —
it only runs in a real deploy, never through TestClient — asserting it delegates
to the inner app and logs nothing.
214 passed (195 baseline + 19).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the hand-rolled media streaming with Starlette FileResponse, drops the
BaseHTTPMiddleware, and enlarges the default threadpool.
3.1 prepare_file_response now returns FileResponse (handles Range/If-Range/206/
416/multipart, sets Accept-Ranges/ETag/Last-Modified, reads efficiently — no
per-64KB to_thread hop that starved the pool). Kept: the early 404 pre-check,
the MIME logic (python-magic + SQLite cache), and every stage-2 behavior — the
temp_* mtime touch (now DEBOUNCED, see below), delete_after -> BackgroundTask
(passed as FileResponse background=), the media_key MIME cache. Removed the
manual Range parsing, file_chunk_generator, and hand-built headers;
Content-Disposition is formed by FileResponse from filename= (no double-set).
206 slices are byte-identical to the old code; accepted RFC-7233 deltas
documented in the tests.
3.2 RequestLoggingMiddleware rewritten as a pure-ASGI class (wraps only send to
observe the status line, never buffers the body, passes non-http scopes
through) — the streaming body flows untouched.
3.3 lifespan sets a larger default executor (ThreadPoolExecutor, IO_THREAD_POOL_SIZE
default 32) and shuts it down on exit.
Review round-1 fixes folded in: the temp_* mtime touch is DEBOUNCED
(TEMP_MTIME_REFRESH_INTERVAL=300s) so FileResponse's mtime-derived ETag stays
stable across a resume/seek session (an every-serve touch broke If-Range resume);
starlette pinned to 0.45.3; the io executor is shut down on lifespan exit; the
ASGI logger includes the query string.
Tests (tests/test_stage3_fileresponse.py, 18): the Range matrix vs FileResponse
with every delta documented; temp_* mtime refreshed when stale AND stable when
fresh (ETag identical); delete_after background runs and removes the file;
media_key MIME cache hit/miss; the ASGI middleware passes the body and logs.
213 passed (195 baseline + 18).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F1 [WARNING] The HTTP_DOWNLOAD_SEMAPHORE admission-control balance was untested,
and it is a plain asyncio.Semaphore — so an over-release would SILENTLY inflate
the permit count and disable the limiter (no ValueError), a green suite hiding the
exact bug stage 2 fixes. Added two tests (mirroring the stage-1 tg_throttle
balance test): the permit is released exactly once when the download errors (count
back to baseline), and a timed-out acquire (503) releases NOTHING (count
unchanged). Adversarially validated: neutralizing the finally release makes the
first test fail, so it genuinely catches a leak.
F2 [low] Corrected the _download_deduped docstring: the detached task sets the
Future in try/except, and its finally ALWAYS pops the key — the previous wording
attributed the Future-set to the finally, which an auditor reading the finally
block would find contradicted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes flaky static media serving: partial files served as 'ready', truncated
stubs living for an hour, and app-wide hangs under a saturated download path.
2.1 _download_atomic(file_id, final_path, timeout): downloads to a unique
{final}.part.{hex}, validates size>0, and publishes solely via os.rename
(atomic on POSIX); a finally ALWAYS removes the partial (timeout/cancel/
zero-size/race-loser). Every downloader call routes through it, so a file at a
FINAL name ({fid} / temp_{fid}) is GUARANTEED complete (grep-proven: the only
safe_download_media caller passes a .part. path). Big-video timeout scales with
size (min/max/min-speed knobs, documented in dockercompose.yml). The sweeper
regex now matches both .part. and legacy .tmp. stubs.
2.2 In-flight dedup registry: the first request for a key runs the download in a
DETACHED task sharing a Future; the task's finally sets the Future AND pops the
key. Waiters await asyncio.shield(fut) so a client disconnect / waiter timeout
cancels only the waiter, never the download — no hung waiters, no stuck key, a
failed download frees the key for retry.
2.3 FloodWait->429: handler before except RPCError (FloodWait subclasses it),
Retry-After = min(value + rand(1,30), 300); propagates from the detached task
through the Future.
2.4 Serving a temp_* file touches its mtime so the 1h sweeper can't delete a
video out from under a viewer.
2.5 The HTTP download semaphore acquire is bounded (wait_for 30 -> 503 +
Retry-After); the permit is released only if the acquire succeeded. The
request-scoped-permit trade-off (a disconnect can transiently exceed the
download count) is documented inline for stage-7 prod observation.
Tests (tests/test_stage2_static.py, 13): atomic publish/clean on every exit incl.
FloodWait-through-the-finally; concurrent big-video serves no partial; dedup runs
one download, a cancelled waiter doesn't hang others, a failed download frees the
key; FloodWait -> 429; mtime touch; sweeper cleans .part./.tmp./stale temp_ but
keeps fresh files. 193 passed (180 baseline + 13).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
F1 [WARNING] pytest-asyncio was not declared, so the 6 new async stage-1 tests
ERROR on a clean checkout (async def not natively supported) — zero regression
protection, masked by a globally-installed plugin. Added pytest-asyncio to
requirements.txt + asyncio_mode=auto to tests/pytest.ini. Verified on a fresh venv
from requirements alone: the tests collect and run (180 passed).
F2 [WARNING] The /raw_json endpoint's get_messages was the one remaining live RPC
without a timeout, violating the stage-1 DoD ('every Telegram RPC is bounded').
Wrapped it in asyncio.wait_for(..., 30) mirroring PostParser.get_post. (It is not
under the tg_rpc gate, so its blast radius was one request, not the app.)
F3 [WARNING] The worker test globally no-op'd asyncio.sleep, so the dedicated
except FloodWait branch was indistinguishable from the generic handler — deleting
it kept the test green. The sleep stub now records delays and the test asserts the
FloodWait backoff of 6 (=min(1+5,900)), distinct from the success path's 2.
F5 [low] The tricky 'gate outside, timeout inside' nesting was open-coded at 3
sites (each re-deriving the invariant). Extracted tg_rpc_bounded(timeout) into
tg_throttle (using asyncio.timeout()); the 3 sites now use it, so a future call
site cannot silently wrap the gate entry and reopen the hang-under-backpressure.
F4 [low] Documented TG_RPC_TIMEOUT in the dockercompose.yml environment block
next to the other TG_RPC_* knobs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Eliminates the app-wide hangs where one stuck Telegram RPC holds the single
tg_rpc gate permit forever (freezing every RSS/HTML request) and where the
background download worker dies silently.
1.1 Every RPC under the global gate is now bounded by asyncio.wait_for
(config tg_rpc_timeout, env TG_RPC_TIMEOUT, default 60). The wait_for wraps
ONLY the RPC body, never the gate acquire, so queue backpressure stays
legitimate; the paginated get_chat_history is collected in an inner coroutine
so the async-for can be bounded. A timeout propagates out of the gate so its
permit is released (no leak).
1.2 The other live RPC paths get the same treatment: _reply_enrichment's
get_messages now runs under the gate + timeout; PostParser.get_post is bounded
at 30s (and a stray print removed); /health's get_me at 10s.
1.3 background_download_worker: queue.get() moved out of the try so task_done()
in finally balances exactly one get (no ValueError that killed the worker);
FloodWait is caught BEFORE the generic Exception (sleeps, does not flood);
download_new_files uses put_nowait so a full queue no longer blocks the
sweeper forever.
1.4 Both background tasks run under a _supervised() wrapper: a crash or an
unexpected return is logged CRITICAL and restarted (restarts rate-limited to
once per 60s so a hard-failing task can't spin), while CancelledError is
propagated to the child for a clean shutdown.
Tests (tests/test_stage1_hangs.py): gate timeout releases the permit + a second
call succeeds; gate cancel mid-spacing releases the permit; the worker survives
Exception and FloodWait with balanced task_done; and _supervised restarts a
crashing task (rate-limited) and an unexpected return, and propagates cancellation
to its child. 180 passed (174 baseline + 6).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Introduce `tg_watchdog_heartbeat_every` to emit periodic INFO heartbeats.
Add cumulative diagnostics counters and richer logging for watchdog probes,
restart reasons, and disconnect‑flap triggers. Update defaults and tests
accordingly.
Introduce an active watchdog that probes the Telegram client to detect
zombie sessions and restart them in‑process. Add configurable disconnect
flap detection with a sliding window to trigger restarts after repeated
disconnects. New environment variables and config entries are added, and
the Kurigram dependency is now version‑pinned.
Replace MagicMock based text/caption mocks with a lightweight StrWithHtml
subclass that provides the required `.html` property while behaving like a
regular string. This fixes length and strip checks in the parser and
simplifies test setup. Also adds default proxy settings to mock_config for
test completeness.
agent_coder
claude code agent
vvzvlad