Add bot avatars and improve client IP handling - #63
Conversation
There was a problem hiding this comment.
Thanks for this PR. I reviewed the exact head commit cecb1446e5b27df75c87c9681dba393126e74e09, imported its diff into the current private canonical tree, and ran both the focused package tests and go test ./.... Those tests passed, and the GitHub checks are green.
I am requesting changes because the following issues need to be resolved before this can be accepted.
1. [Blocking] Bot avatar seeding is not idempotent under errors or concurrent startup
In internal/app/files/botavatars/botavatars.go, an error from CurrentProfilePhotoKind is ignored and seeding continues. A transient database error can therefore be treated as if the avatar does not exist.
The check, photo creation, and profile-photo binding are also not atomic. If two server instances start at the same time, both can observe no current avatar, create different random photo IDs, and call AddProfilePhotoKind. The PostgreSQL implementation currently derives the next order with MAX(sort_order) + 1 without a lock or a uniqueness constraint for the current photo. This can create duplicate active rows with tied order, make the selected current photo nondeterministic, and leave orphan photo/blob records.
Please make this path fail on read errors and enforce idempotence at the storage boundary. The solution should remain correct with multiple server instances, for example through a transaction plus locking/advisory locking or a suitable uniqueness/upsert invariant. It should also avoid creating media that becomes orphaned when another instance wins the race.
2. [Blocking] MTProto Edge now has a reverse dependency on the complete RPC business layer
The client-IP behavior itself is small and useful:
net.Conn.RemoteAddr -> mtprotoedge connection metadata -> RPC request context -> AuthKeyClientInfo.IP -> persisted authorization.ip -> session/admin display
However, internal/mtprotoedge/inbound_rpc.go imports internal/rpc only to call rpc.WithClientIP. This reverses the intended architecture. Edge is the protocol/session boundary and should produce transport facts such as the remote IP; it should not depend on the RPC business implementation.
This is a compile-time and transitive dependency problem, even though Edge does not directly instantiate every business service. In this PR, go list -deps ./internal/mtprotoedge grows from 298 packages to 465 packages and pulls in internal/rpc plus business packages including auth, contacts, files, phone, users, read models, projections, peer views, and secret chats.
That coupling prevents Edge from remaining independently buildable, makes protocol code sensitive to RPC/business changes, and works against the planned Edge/Core boundary.
Please keep the IP as neutral transport metadata and translate it into the RPC context outside mtprotoedge. Reasonable options include:
- wrap the RPC handler at the composition root and call
rpc.WithClientIPthere; - define a small neutral request-metadata type at an appropriate boundary; or
- extend the handler boundary to accept transport metadata without importing
internal/rpc.
The key invariant is that internal/mtprotoedge must not import the full RPC package.
3. The Premium bot avatar ignores the configured Premium bot ID
The avatar map uses domain.PremiumBotUserID directly, while the server supports a configured identity through domain.PremiumBotConfiguredUserID() and TELESRV_PREMIUM_BOT_USER_ID. The seeder also runs before EnsurePremiumBotIdentity.
With a custom Premium bot ID, startup can attach the avatar to the default or retired identity while leaving the actual configured Premium bot without an avatar. Please resolve and seed the configured identity, and ensure ordering is consistent with identity creation.
Tests and cleanup requested
Please add focused tests covering:
- an existing avatar is left unchanged;
- a
CurrentProfilePhotoKinderror fails rather than writing; - concurrent or multi-instance seeding creates exactly one current avatar and no orphan media;
- a configured Premium bot ID receives the avatar;
- the real connection-to-persistence path carries the client IP from the accepted connection through RPC routing into
authorizations.ip.
The added router test verifies context propagation only after the IP is already placed in the context; it does not cover the new Edge-to-RPC integration or persistence path.
Finally, please run gofmt on cmd/telesrv/main.go; its import grouping is currently not gofmt-clean.
Once these points are addressed, I can review the updated commit.
…rs, verifybot, gif, premiumbot) Embedded avatars for the system account (777000) and the official bots, and added a startup seeder that assigns them. The avatar images are sourced from original Telegram and are not branded.
The admin panel hid the avatar-change button for system bots and the backend rejected avatar updates for IsSystemUserID accounts. System bots in the admin UI map to the built-in service accounts, so allow the same avatar update path used for regular users/bots.
- Don't ignore CurrentProfilePhotoKind errors (log + skip peer) - Wrap entire Seed loop in BeginTx with pg_advisory_lock to serialize concurrent instances and prevent orphan media - Add WithTx to MediaStore interface for transactional operations - Add BeginTx to AvatarSetter interface - Add tests: skip existing, read error, concurrent, beginTx failure
…eation - Use domain.PremiumBotConfiguredUserID() instead of the compile-time PremiumBotUserID constant so TELESRV_PREMIUM_BOT_USER_ID deployments get the avatar on the right (configured) identity - Run botavatars.Seed after EnsurePremiumBotIdentity so the configured identity exists before its avatar is attached - Add test covering configured premium bot ID usage - files.Service.BeginTx now returns botavatars.AvatarSetter (import botavatars from files package; no import cycle)
The test released the blocking backend as soon as the first (leader) caller reached its range read, but did not ensure the other 15 callers had already entered the singleflight. A straggler arriving after the leader's read completed would start a second singleflight generation, observe its own byte backing, and spuriously fail (~10-30%). Now all callers are launched from a barrier and the test waits for every caller to enter GetFile before releasing the blocked leader, and Gosched lets remaining callers park on the singleflight while the leader is still blocked. Verified reliable with -count=200 and under -race.
- botavatars: Seed now returns error; a CurrentProfilePhotoKind read error, asset read error, create error, or bind failure aborts the whole seed instead of being skipped/logged. The check+create+bind sequence runs inside the advisory-locked transaction (SeedTx), so any failure rolls back media and bindings, leaving no orphan photo/blob rows even under concurrent or multi-instance startup. - files.Service: replace the broken BeginTx channel handshake (which deadlocked the production path and ignored errors) with SeedTx, running fn inside the MediaStore.WithTx transaction. WithTx now uses pg_advisory_xact_lock so the serialising lock is released automatically on commit or rollback. - mtprotoedge: client IP is carried as neutral transport metadata via the new internal/transport package instead of depending on internal/rpc. rpc's WithClientIP/ClientIPFrom delegate to the same neutral carrier. This removes the reverse dependency on the RPC business layer and drops the edge dependency graph back below internal/rpc. - main.go: seed Premium bot via configured identity after EnsurePremiumBotIdentity; handle Seed error; gofmt import grouping. - tests: focused unit tests (existing avatar unchanged, read/create errors fail without writing, concurrent seeding yields exactly one current avatar and no orphan media, configured Premium bot ID), a postgres storage-serialization integration test, and an mtprotoedge e2e test proving the client IP flows from the accepted connection through RPC into authorizations.ip.
|
Summary of changes addressing review
|
|
Thanks for the updates. I re-reviewed the exact head commit The previous findings are substantially addressed:
The focused tests, One blocking startup issue remains: [P1] The transaction-scoped files service drops dependencies required by the MP4 avatar pathIn The Premium bot seed calls
This is also the exact failure reported by the current Please make the transaction-scoped avatar adapter retain every configured dependency needed by the avatar pipeline while replacing only the media store with its transactional view. In particular, preserving only Please also add a regression that invokes Small non-blocking cleanup: Re-review verdict remains BLOCK until the startup path succeeds and the Docker topology check is green. |
SeedTx previously built a new Service carrying over only a subset of fields (media, blobs, dc, log, caches, uploadQuota), dropping uploadParts, thumbs, gifs and others. As a result seeding bot avatars failed with "upload part backend not configured" (CreateAvatarVideoFromBytes -> SaveFilePart depends on uploadParts). The whole service is now copied and only media is swapped for the transactional store. Added an integration test running botavatars.Seed through a real files.Service with a real LocalFS backend (not a mock); fakeMediaStore now persists profile photos so the seed can be verified. Also fixed gofmt in router_dispatch_test.go.
c1d7ffd to
be40cec
Compare
The transaction-scoped service created by SeedTx now inherits every configured dependency needed by the bot-avatar pipeline (uploadParts, thumbs, gifs, gifCatalog, mapTiles, externalMedia, webpage, effects, premiumPromo and the read-model caches) and swaps in only the transactional media store. Previously uploadParts and thumbs were dropped, so on a fresh deployment the Premium bot's animated-avatar seed failed at SaveFilePart with "upload part backend not configured" and, even if staging were preserved, the video still would have fallen back to a generated placeholder instead of extracting the frame. The singleflight groups and premiumPromo mutex are deliberately re-initialised fresh because copying a lock after use is unsafe (go vet). This replaces the earlier whole-struct copy with an explicit carry-over. Add a regression that runs botavatars.Seed through a real files.Service using separate permanent-blob and upload-staging backends, asserting the staging backend was actually used and the video thumbnailer was invoked (i.e. thumbs survived the transactional wrapper). countingUploadPartBackend gains PutUploadPart/DeleteUploadPart counters to observe staging writes.
|
Fix P1: transaction-scoped files service now carries the avatar pipeline
|
bytefakers
left a comment
There was a problem hiding this comment.
Re-reviewed at 9cffa06. The previous blockers are resolved: transactional bot-avatar seeding preserves the required upload/thumbnail pipeline dependencies, PostgreSQL advisory locking and idempotency were verified, Edge now carries client IP through neutral transport metadata without depending on RPC or app packages, and the full client-IP persistence path is covered end to end. Targeted tests, race checks, real-PostgreSQL concurrency tests, go vet, the full Go suite, boundary/leak checks, and all exact-head CI jobs passed. Approved.
iamxvbaba
left a comment
There was a problem hiding this comment.
Re-reviewed at 9cffa06 after the requested changes. The previous blockers are resolved: bot-avatar seeding now preserves the required transactional service dependencies and is serialized across instances, the configured Premium bot identity is handled in the correct order, Edge carries the remote IP through neutral transport metadata without RPC/app-layer dependencies, and the connection-to-authorization persistence path is covered end to end. Targeted tests, race checks, repeated real-PostgreSQL concurrency tests, go vet, the full Go suite, boundary/leak checks, and all exact-head CI jobs passed. Approved for merge.
|
Integration completed.
The accepted public code and asset files were imported into the current canonical tree without a functional rewrite; the only canonical additions are private compatibility and persistence notes. The canonical change passed targeted and full Go tests, full vet, the Admin production build, race coverage, repeated MTProto client-IP E2E coverage, and repeated real-PostgreSQL avatar-seeding concurrency coverage. Contributor attribution is preserved in the public merge history and in the canonical commit co-author trailer. Thank you for the contribution. |
Embedded avatars for the system account (777000) and the official bots, and added a startup seeder that assigns them. The avatar images are sourced from original Telegram and are not branded.
Update:
In the last three commits, I made the following changes:
Persisted the client IP address in device authorizations so it is displayed in the Admin Panel.
Updated the stored client IP on every reconnect, not only during login.
Allowed changing avatars for system bots from the Admin Panel.