Repository navigation
fix(delivery): one dispatch per job — fence lease, album ambiguity, bot-route media rescue, staged-file hygiene - #341
Merged
Conversation
added 8 commits
November 6, 2026 04:03
…NT is terminal A CLAIMED fence whose worker_id matched the caller was handed straight back as dispatch with no lease check: every goroutine in a worker process shares one worker id, so any dispatch that outlived the 30s job lease (a large album re-upload) was re-claimed by a sibling goroutine mid-flight and the job was delivered twice. FenceClaim now reads the pre-upsert state inside the transaction — live same-worker claim = in_flight, expired = stale, FAILED re-claim (the replay path) preserved. The SQLite fence writers also carried no SENT guard (the PG store and the Python repository both do): a late sibling failure could stamp FAILED over a confirmed send and turn a delivered message back into a re-dispatchable job. Both writers are now guarded. Regression tests: TestFenceClaimSameWorkerHonorsLease, TestFenceSentIsTerminalOnSQLite, and the PG mirror TestPostgresDuplicateFenceClaimSameWorkerNeverRedispatchesLiveLease (the old same-worker-redispatches test pinned the duplicate).
… to the account; staged files cleaned up Three delivery defects from the media-recovery audit: - dispatchAlbum fell into the per-item re-upload loop on ANY group-copy failure — including an ambiguous one whose upload Telegram may already have accepted — delivering the album twice (the Python path shares the group-stage behavior, but the port also returned the LAST item id, so mapping/edit-sync/auto-delete acted on the wrong message, and the fallback opt-out was honored only for NeedsReupload classes). An ambiguous outcome now records UNKNOWN, the opt-out is honored for every class, and the mapping keys on the FIRST delivered item as the Python contract states. - A bot-route media copy refused with a permanent 400 (restricted source, target-side media rejection — the Bot API adapter reports BOT_API_400 so NeedsReupload can never match) failed the job permanently. The legacy Python path rescued these through the account transport; botMediaRefused restores that rescue behind the fallback_enabled opt-in. - reuploadMedia never deleted its staged scratch file (the Python path deletes its temp files; the port dropped that and MediaQuotaBytes is referenced by nothing). The file is now removed whichever way the attempt ends. The fake gateway gained SendMediaFailsWith and DownloadWritesFile so upload-time failures and the file lifecycle are actually testable. Six regression tests in recovery_gaps_test.go pin each sequence.
A gRPC Unavailable that carries 'Error while dialing' proves no HTTP/2 stream ever existed — nothing reached a handler, so no Telegram operation can have been attempted. Production stranded 3 such rows UNKNOWN forever because the marker list accepted only Unimplemented. A bare Unavailable still stays UNKNOWN: a transport that dies mid-upload reports the same code after Telegram accepted the send.
Jobs purge after 7 days; their fences lived forever — the production audit found 127 orphaned SENT fences (including one that collides with a later live-fire run's job). PurgeAgedFences deletes terminal (SENT/FAILED) fences older than the same 7-day window on an hourly gate; UNKNOWN fences are never swept — they are unresolved evidence.
The replay rebuild inserted no job_kind (a keyword_reply entry came back as kind=forward) and the kind-less lookup could reset a forward sibling that shares (rule, source, message, target). On PostgreSQL the rebuild was dead code outright: a 4-column ON CONFLICT target against the 5-column delivery_jobs_unique constraint — PostgreSQL rejects the INSERT, every replay of a purged row returned replay_failed, and the mapping-placeholder restore never ran. The conflict target now names all five columns, the INSERT carries job_kind, and both stores derive the kind from the entry's payload.
…worker With ATF_WORKER_ENABLED=1 the Go worker's reconciler is the single authority over UNKNOWN fences/jobs; the Python check kept re-flagging the same rows every sweep (production warned about the 3 e2e artifacts every pass) — a second voice over state it does not resolve. The check stands down when the worker owns delivery; every other check keeps running.
The test derived its window from the machine-local zone while IsQuietNow reads the account's timezone offset (0 by default = UTC): red on any non-UTC host, green on UTC CI. Both gates now use the same clock the model reads.
…y runbook, artifact manifest, live-test spec - progress ledger: every CSV finding id mapped to evidence, fix and status - recovery runbook: conditional fail-closed requeue (executed for the 12 PHOTO_EXT_INVALID rows on 2026-11-06; 12/12 delivered exactly-once) and the CORRECTED artifact-removal statement — the previous e2e-go-% pattern matched only 1 of the 3 artifacts (e2e-go/e2e-go2/e2e-go3) - artifact manifest: all three e2e-go rows investigated, nothing deleted - live-test spec: the controlled re-upload verification for the future authorized run
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. |
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.
Problem and root cause
The phase-2 media-delivery audit (independent adversarial review + production DB snapshot) confirmed seven delivery-safety defects, three of them P1, each with a constructed failure sequence:
FenceClaimreturned dispatch for any CLAIMED fence whoseworker_idmatched the caller — and every goroutine in a worker process shares one worker id. Any dispatch outliving the 30s job lease (a large album re-upload) was handed to a sibling claim mid-flight → the same message sent twice. The inherited-then-dropped lease contract; the PG mirror test actually pinned the duplicate ("SameWorkerRedispatches").FenceRecordOutcome/FenceRecordSentoverwrote a SENT fence (PG + Python both guard) — with TASK 01-02: Repository audit + Telegram-native error forensics #1 it converts a confirmed send back into a re-dispatchable FAILED.CopyMediaGrouptimeout after Telegram accepted fell into the item loop and delivered the album twice; the port also mapped the LAST item id (Python maps the first — auto-delete/edit-sync keyed the wrong message) and honored the fallback opt-out only for rejection classes.BOT_API_400, soNeedsReupload()can never match there; legacy Python rescued restricted-source media through the account transport, the port dropped it./tmp/atf-media/<job>files behind on every attempt;MediaQuotaBytesis dead config.Unavailable(P2) — 3 production rows UNKNOWN forever; dial-phase evidence proves no handler ran.ON CONFLICTagainst the 5-column unique; every purged-row replay failed, mapping restore never ran; both stores also rebuilt keyword_reply entries asforwardand could reset a forward sibling.Production-integrity findings handled alongside (evidence in
docs/reliability/phase2-progress-ledger.md):PHOTO_EXT_INVALIDjobs requeued and delivered exactly-once (conditional fail-closed requeue per the runbook; dry-run gates 12/0/0; 12/12 SENT, 0 WARN/ERROR).ATF_WORKER_ENABLED=1.e2e-go*artifacts: manifest produced; the prior runbook'se2e-go-%pattern matched only 1 of 3 (corrected explicit-ID statement); deletion remains authorization-gated.Exact code paths changed
worker/internal/queue/sqlite.go,postgres.go— FenceClaim pre-read state machine (live same-worker = in_flight, expired = stale, FAILED re-claim preserved), SENT-terminal guards,PurgeAgedFences.worker/cmd/worker/main.go— hourly fence-retention sweep.worker/internal/engine/delivery/delivery.go,branding_stage.go— album ambiguity guard + first-item mapping + opt-out;botMediaRefusedUSER fallback; staged-filedefer os.Remove.worker/internal/engine/reconciler/reconciler.go— dial-phase marker.core/infrastructure/persistence/{sqlite,postgres}_repositories.py— kind-aware DLQ replay, 5-col conflict target.core/application/delivery_reconciler.py— worker-ownership stand-down.worker/internal/domain/autoreply/autoreply_test.go— timezone-robust quiet-hours gates.worker/internal/telegraph/fakegateway/fake.go—SendMediaFailsWith+DownloadWritesFileknobs (upload-time failure was previously untestable).Tests: existing reused, new added, actual results
guarantees_test.go, reconciler marker table, runtime contract tests.fence_lease_gaps_test.go(3),recovery_gaps_test.go(6), extended marker table (2 cases), rewritten PG same-worker test, PG scratch-db suite, Python stand-down test.-racegreen; Python suite 4292 pass / 27 skip (1 environmentalis_checkoutfailure: git-archive export has no real.git; CI checks out properly); real-PostgreSQL 17 queue suite green (ephemeral container through SSH tunnel, destroyed after).SQLite/PostgreSQL validation
Fence semantics restructured identically in both stores; the PG queue suite ran against a real server (not skipped) this round. No schema changes; no new columns/tables.
Backward compatibility
Fence outcomes shift only for the duplicate-producing paths. Same-worker live-lease claims now defer (they previously double-sent); FAILED re-claim, replay, and recovery flows are preserved and pinned by tests. The reconciler's auto-close set grows by the dial-phase marker only.
Production operational restrictions
/tmp/atf-backup-pre-requeue.db, conditional transaction, expected-count assert, post-verify, rollback documented). Row-level evidence preserved via therequeued-bymarker.docs/reliability/phase6-live-test-spec.mdand NOT executed.Approval-gated outstanding work