Repository navigation
fix(delivery): PHOTO_EXT_INVALID root cause — re-upload staging now carries the real file extension (+ fallback extended to target-side media rejection) - #339
Merged
Conversation
…iggers the re-upload fallback 12 real jobs on rule 40edad5a failed PERMANENTLY with PHOTO_EXT_INVALID and were never retried or recovered: the copy/re-upload fallback only ever asked IsForwardingRestricted(), so a media-rejection error — where the copy is PERMITTED but the target chat cannot accept the copied media's extension — went straight to FAILED. PHOTO_EXT_INVALID is a per-message incompatibility, not a permission problem: the copy preserves a file reference the destination rejects, and retrying the same copy can never fix it. The recovery is exactly what a forwarding restriction already does — download the media and upload it fresh, which normalizes extension and dimensions. NeedsReupload() now covers both reasons a copy cannot survive as-is (source forbids it, or target rejects the payload) at all ten copy call sites; the task's fallback_enabled flag still gates both, and the error vocabulary gains the neighbouring MEDIA_INVALID / PHOTO_INVALID_DIMENSIONS / VIDEO_NOT_MODIFIED family. Tests pin: the classification (restriction, media rejection, and the not-a-reupload cases — flood wait, network timeout, unknown outcome), the end-to-end recovery (media-rejected copy with fallback_enabled delivers via DownloadMedia), and the opt-out (no fallback = no re-upload). FakeGateway gains CopyMediaRejected so the defect's exact production shape is reproducible without a restricted channel.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…PHOTO_EXT_INVALID root cause The first commit's diagnosis was incomplete. Production jobs failed with 'caused by messages.SendMedia' PHOTO_EXT_INVALID — the re-upload fallback DID run for those jobs; what killed them was the staging step itself: the scratch file was written as bare 'job-<id>-media' with no extension, and Telegram infers a photo's MIME type from the staged file's name. The fallback could never succeed, so the job died permanently either way. stagedExtension() now asks the runtime for the source media's real extension (GetFileExtension — fully implemented end to end but never wired) and stages under <scratch>/<job>.<ext>: - runtime answer is normalized (lowercase, dot-prefixed, [a-z0-9] only, length-capped) — untrusted input never becomes a path surprise; - when the runtime cannot answer (source gone since the copy was refused, or a kind with no inferable name), the ingest classification supplies the canonical extension (.jpg/.mp4/.ogg/.mp3/.webp); - documents and unknown kinds stay extension-less on purpose: send_document accepts bare files and guessing would mislabel them. No retry-loop risk: SendMedia failures keep their terminal classification, so a genuinely broken media file still dead-letters instead of looping download→stage→send. Tests pin: the staged path carries the normalized extension and PrepareMediaUpload references the exact file DownloadMedia wrote; the classification fallback; the document opt-out; the hostile-suffix rejection; the probe-failure fallback; plus the sanitizer's full table. Python gains the contract suite for get_file_extension the Go side now trusts (photo=.jpg, raw file_name suffix, typed error on failure).
Opselon
changed the title
added 7 commits
November 5, 2026 20:49
…s deliveries The fence contract says an ambiguous outcome 'goes UNKNOWN and reconciliation decides' — but the decision step never existed: no caller of FenceResolveUnknown anywhere in the module, and production stranded 13 UNKNOWN jobs for 20+ hours (10 of them since Nov 4, all with the same evidence: 'rpc error: code = Unimplemented desc = Unexpected <class NotImplementedError>' — the pre-#330 missing-SendMedia era rejecting the call server-side before any handler ran). The reconciler is that decision step. Every pass it lists UNKNOWN fences older than twice the claim lease and closes only the ones whose recorded failure PROVES no Telegram operation was attempted — gRPC Unimplemented means the runtime server rejected the method before any handler ran, so the message cannot be in the target and closing to FAILED is safe. Every genuine ambiguity (timeouts, dropped connections, unknown outcomes) stays UNKNOWN and visible in /ops: guessing there is exactly the duplicate delivery the fence exists to prevent. Store surface: UnknownFences (the reconciler's input, both backends) and ReconcileUnknown — one transaction closing fence UNKNOWN->FAILED and job UNKNOWN->FAILED together, both status-guarded, rolling back rather than committing a half-reconciliation (a FAILED fence beside a SENT job would let a duplicate dispatch through; the guard is tested). Configured by ATF_RECONCILE_INTERVAL_SECONDS (default 60s), grace = 2x lease; runs beside the existing 15s fence sweep in cmd/worker. Tests pin: the aged-UNKNOWN-only listing (limit, ordering, evidence carried), the atomic closure with the audit reason on both rows, re-dispatchability after reconciliation (FenceDispatch), the moved-fence no-op, the inconsistent-pair rollback, the grace window, and the full evidence-classification table.
…ject-aware metrics Three evidence-backed repairs from the production audit (ledger W2/G1/F2): 1. DLQ replay of a purged row rebuilt a forward job WITHOUT its message_map placeholder. Retention purges mappings after 30 days while dead-letter entries persist, and the worker's NoteMapping only UPDATEs (by design — replies must never touch the forward's row), so the confirmed target id would be silently dropped and edit/delete sync would never learn the delivery happened. Both replay paths (SQLite + PostgreSQL) now restore the placeholder inside the replay, with the enqueue path's own never-regress upsert shape (a surviving SENT row is never flipped back) and the reply-key guard. 2. Paid-addon entitlement keys: the bot catalogue rows and the web dashboard record the short spellings (kw_reply, auto_del, affiliate) while the engines gate on the canonical vocabulary (keyword_reply, auto_delete_message, affiliate_kit). Three paid add-ons never fired from any real client — the canonical keys existed only in tests. The single enforcement point on each side (ForwardRule.is_addon_enabled / ruleAddonEnabled) now honors both spellings via an alias table; entitlements recorded by any client fire, canonical keys keep working, and the strict server-side enforcement itself is unchanged. 3. Deliberate non-deliveries (attached-filter drop, task-level block_links, forward-every-N cadence skip) were counted as 'failed' — the ops panel degraded a healthy task for exercising its own policy, and a filter drop also failed the filter stage. They now count under a distinct 'rejected' outcome (Collector + /healthz + /ops snapshots); the filter drop no longer marks the stage failed. The job rows keep their designed FAILED audit-trail state, pinned by the existing tests. Tests: the mapping-restore contract (placeholder restored, worker's UPDATE-only success write lands, SENT mapping never regresses, reply replays never touch the forward's row), the alias table on both engines plus the keyword-reply projection through the alias, and the reject-vs- fail outcome accounting at all drop sites.
… redaction Observability-audit repairs (ranked gaps from the full sweep): - jobLogger now carries worker_id on every delivery line — multi-replica log attribution worked only through /ops before. - failureToRing keeps correlation_id: the /ops error ring stored code/error_type/rule_id/job_id but dropped the one id that ties the failure to its end-to-end trace. - handleFailure records the /ops end-to-end outcome on every branch (unknown/retried/failed), exactly as Process's own contract comment already promised — the latency ring was systematically blind to every failure and defer path, so p99 read fast because it only ever saw sends. ensureBrandingStage1 threads jobStart so its failure path reports the true elapsed time, not a truncated one. - RULE_AUTO_PAUSED drops now record the outcome like their RULE_DISABLED sibling (they marked the job but skipped the metrics observation). - The AI stage counts honestly: the transform defer no longer records 'ai' for every text job (most carry no rewrite — the stage read healthy-with-usage it never had, and rewriting jobs were counted twice, once per attempt by the AIMetricsObserver plus once here); main.go enables 'ai' only when a transformer is actually wired instead of blanket-enabling all stages. - The stdout JSON sink applies the SAME redaction as the central client (secret-shaped keys and content fields → [REDACTED], secret-shaped substrings in messages masked): the container stdout is what a log collector scrapes, and until now it had no redaction at all. - The reconciler's log lines carry worker_id/rule_id/source_message_id. - /healthz no longer double-probes the DB, and the 15s ticker probes it too: db_connected, pending depth and derived health state are now truthful on a healthy worker nobody has polled yet, instead of db_connected=false / degraded until the first readiness request. - CI python-tests runs pytest with -rs: every skipped test prints its skip reason — a skipped suite must be inspectable, not just countable. Test repairs: ORD-002 accepts the filter stage's policy-skipped observation (the drop contract changed to rejected/skipped in the previous commit; the ordering contract — filter before any transform — is unchanged and still asserted), and the DLQ-replay tests construct the real pre-state (placeholder row exists before the success write marks it SENT) through the mapping repository that actually owns update_delivery.
… full footprint Two production-growth and contamination repairs: 1. ingestion_gaps retention (production reached 468 rows): terminal (FAILED/RESOLVED) gap rows are deliberate 'visibly, never silently' loss records — but not exempt from bounded storage. A cleanup() on both backends deletes terminal rows older than the same 30-day window the message map applies to delivery confirmations; open rows (PENDING/RETRYING) are live recovery work and are never touched, whatever their age. The channel poller runs it hourly alongside its gap-recovery sweep, on the same cadence as the durable queue's retention pass. 2. worker/test/live/run_matrix.py runs against the production database by design, and its cleanup stopped at rules + counters — deliberately keeping delivery_jobs (keep_jobs=True), which is what seeded production with orphaned test rows (the e2e-go-* UNKNOWNs the queue still carries). drop_rule now purges the replica's ENTIRE footprint — delivery_jobs, delivery_attempts fences, message_map rows, dead letters, counters, the rule — and refuses any id that is not one of its own mtx-* replicas: a non-replica id there is a caller bug, not a cleanup target. The outcome is captured before cleanup, so nothing observable is lost. Tests pin: aged FAILED rows purge, aged OPEN rows never do.
…ature matrix, DB diagnostics, CI history, approval-gated recovery procedure
…trap; log-audit repairs Production log audit (5955 WARN/ERROR rows from the LIVE Rust logger store + diagnostic_events, deduplicated to 80 signatures) surfaced the real defect behind the gap backlog: single-id recovery built GetChannelDifference from GetFullChannel's pts. GetFullChannel is flood-limited (275 rate-limit waits in one weekend) and its failure was silently swallowed into pts=0 — Telegram then rejected EVERY recovery attempt with PERSISTENT_TIMESTAMP_EMPTY (450 occurrences), burning each gap's retry budget on a request that could never succeed. The posts the gap machinery exists to save were lost anyway. Recovery now fetches the id with channels.getMessages: one call, no pts, no flood-sensitive full-channel round-trip, and Telegram's own MessageEmpty answer for deleted ids. The regression test pins the RPC contract scoped to the recovery window (GetMessages used; GetFullChannel/GetChannelDifference never called there), including the deleted-id path and dispatch of recovered posts. Log-volume repairs from the same audit: - an exhausted gap is ONE authoritative structured event now — the duplicate logger.error tripled every terminal row (473+463+463) for one fact; - the delivery reconciler stops flagging retention-aged orphan mappings: terminal jobs purge at 7 days, mappings keep 30 — an old mapping whose job is gone is cleanup's expected outcome, not a state mismatch (58 identical production warnings). In-window orphans are still findings; - the worker's health server logs shutdown as INFO, not WARN (normal lifecycle, 18 production WARNs), keeping WARN for real failures. The ledger gains the full log-audit section: every deduplicated cluster, its verdict (defect / stale deployment / external / by-design), and the action taken — including the 401-vs-404 evidence that the deployed API image predates #335 (a deployment blocker, not a code bug).
The recovery fetch is channels.GetMessages since the pts trap was removed; two diagnostic strings in the same path still said GetChannelDifference (the exhausted-gap event's error_detail and the permanent-failure event's method attribute). A log line naming an RPC the code does not call is an audit trail that misleads the operator reading it.
This was referenced Nov 5, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Corrected diagnosis (commit 2 supersedes the first hypothesis)
Live evidence: 12 real jobs on rule
40edad5afailed permanently withPHOTO_EXT_INVALID. The terminal error line carriedcaused by "messages.SendMedia"— notmessages.CopyMessage(orForwardMessages).That changes the story. The re-upload fallback DID run for those jobs. What killed them was the staging step itself:
Telegram's
messages.SendMediainfers a photo's MIME type from the staged file's name. Pyrogram writes the download to the requested path as-is (verified live: a JPEG payload staged as/tmp/atf-media/<job>keeps magic bytesff d8 ff e0and no extension), so the upload was rejected withPHOTO_EXT_INVALIDbefore it could ever succeed. The fallback ran and lost; the job was marked permanently failed either way.Commit 1 — still correct, now secondary (d5f81e1)
NeedsReupload()extends the copy-failure fallback to target-side media rejections (PHOTO_EXT_INVALIDand the neighbouringMEDIA_INVALID/PHOTO_INVALID_DIMENSIONS/VIDEO_NOT_MODIFIEDfamily) at all ten copy call sites. A copy that the target refuses must escalate to a re-upload, exactly like a forwarding-restricted source. Regression tests pin the recovery and the operator's opt-out.Commit 2 — the root cause (fea2665)
stagedExtension()asks the runtime for the source media's real extension viaGetFileExtension— an RPC that was fully implemented end to end (pyrogram adapter → gRPC server → Go client → fake) but never wired into any caller — and stages under<scratch>/<job>.<ext>:[a-z0-9]only, length-capped) — untrusted input never becomes a path surprise;.jpg/.mp4/.ogg/.mp3/.webp);send_documentaccepts bare files, and guessing would mislabel real files;SendMediafailures keep their terminalTELEGRAM_PERMANENT_ERRORclassification — a genuinely broken file still dead-letters instead of looping download→stage→send.Tests
Go (
delivery_test.go): staged path carries the normalized extension andPrepareMediaUploadreferences the exact fileDownloadMediawrote; classification fallback; document opt-out; hostile-suffix rejection (path traversal never reaches the staging path); probe-failure fallback; full sanitizer + default-extension tables. E2E: media-rejected copy withfallback_enableddelivers via re-upload; without the opt-in it must not run.Python (
telegram_runtime/tests/test_file_extension.py, new): the contract the Go side now trusts — photo ${'\u2192'}.jpg, rawfile_namesuffix passthrough (normalization is Go's job),""when no name is inferable, typed error on source-read failure.Deployment note
Live verification of the fix requires deploying the rebuilt worker to
vps-root— intentionally not executed, awaiting explicit approval per the task boundaries. The PR remains open and unmerged.