fix: eliminate critical refund bug and harden startup/persistence config - #2
Open
absolutezeroo wants to merge 11 commits into
Open
fix: eliminate critical refund bug and harden startup/persistence config#2absolutezeroo wants to merge 11 commits into
absolutezeroo wants to merge 11 commits into
Conversation
- SEC-01: refund compensation now uses CancellationToken.None and logs before attempting it, so a cancelled purchase can no longer leave a player permanently debited without their item. - RES-01: enable EnableRetryOnFailure on MySQL; adapt the three manual transaction sites (PlayerWalletGrain, GroupDirectoryGrain, RoomGrain.Trading) to run inside a retryable execution strategy with a fresh DbContext per attempt. - CFG-02: remove the dead DatabaseConfig.LoggingEnabled option; add an optional MySqlServerVersion so ServerVersion.Parse can replace AutoDetect (which opens a connection during DI configuration). - DATA-01: remove the duplicate WriteToDatabaseAsync call in PlayerGrain.SetFigureAsync. - HC-08/HC-09: shared appsettings.json no longer ships a Windows asset path or Debug-level logging as the cross-platform/production default. - CODE-02: fix exceptions that passed a message as paramName (PackageHandler, Rc4Engine); fix two lingering nullability warnings (RentableSpaceGrain, DashboardEndpoints) so the build is warning-free. - NET-01/NET-02: PackageHandler's IVortexContextAccessor/IErrorGroupingSink are now required dependencies wired through both network transports instead of silently-null optionals; add a Vortex.packet.dropped metric so packets that vanish during encoding are now visible. - RevisionManager: use a ConcurrentDictionary and throw on a null revision instead of failing silently. - IRoomGrain: mark pure-read methods [AlwaysInterleave] so they don't queue behind the 50ms room tick. - Remove a commented-out UsePingPong() call and switch a bootstrap log line to structured logging. All 564 existing tests pass; build is clean at 0 warnings.
…, enable build phase 2 - ASY-01: replace ~28 unsupervised `_ = ...Async()` call sites with either `await` (message handler responses to the client) or the existing LogAndForget helper (room broadcasts and grain-turn side effects, where awaiting would cost a tick). The two self-supervising heartbeat loops and the LogAndForget helper's own internal fire-and-forget are intentionally left untouched. - ORL-01: remove the PLAYER_STORE/ROOM_STORE grain storage registrations and their constants - no grain ever used [PersistentState] against them; every grain persists through EF Core. Only PubSubStore (used by the stream providers) remains. - Flip VortexAIPolicyPhase to 2 by default now that the build is warning-free, promoting CS8602/CS8604/CS8618/CA2000/CA2012/CA2201/ IDE0058 to build errors. IDE0005 is excluded: it only runs at build time with GenerateDocumentationFile enabled, which would force XML doc comments repo-wide against this project's own style. All 564 tests still pass; build is clean at 0 warnings under phase 2.
…ain call - ARCH-01: introduce IReferenceDataProvider (LoadStage + ReloadAsync). All 14 reference-data providers (furniture, catalog x2, club offers/ gifts, currencies, badge parts, marketplace settings, pet palettes/ commands/levels/vocals, navigator, room models) now implement it and are additionally registered under the new interface, resolving to the same singleton instances. VortexEmulator goes from 18 constructor dependencies to 3 (logger, IEnumerable<IReferenceDataProvider>, INetworkManager), loads providers in parallel by stage instead of one giant sequential await chain, and adding a new reference-data source no longer touches Vortex.Main at all. Catalog snapshot providers and the club gift provider stay in stage 1 since they read furniture definitions during reload, so furniture (stage 0) must finish first; confirmed no other cross-provider dependencies exist. - REV-01 (registration half): revisions are now registered by contract (IRevision) and discovered via IEnumerable<IRevision> from a small dedicated RevisionRegistrationService, instead of VortexEmulator taking a concrete Revision20260701 constructor parameter and calling RegisterRevision directly. Added IRevisionManager.SetDefault and an optional Vortex:Revisions:DefaultRevisionId config key so the default revision no longer depends on registration order by accident. The larger REV-01 items (RevisionBase inheritance, per-domain revision maps) are still open. - PERF-01: MessageRegistry.CreateContextAsync no longer makes its own GetActiveRoomAsync grain call. MessageSystem.PublishAsync already resolves the active room for tracing/metrics and opens an ambient IVortexContextAccessor scope carrying it before invoking the pipeline; CreateContextAsync now reads that instead of repeating the grain round trip, cutting inbound-packet grain calls from 2 to 1. All 564 tests pass; build stays clean at 0 warnings.
- SEC-06: in CatalogPurchaseGrain (direct purchase and gift purchase), reorder the compensated purchase callback so the reversible step (tracking credit spend) runs before the irreversible one (granting the inventory item), and move the CatalogPurchasedEvent/ CatalogGiftPurchasedEvent publishes outside the compensated scope entirely so a failing event subscriber can no longer trigger a refund of an already-completed sale. Checked every other ExecutePurchaseAsync call site (Marketplace, LtdRaffle, RentableSpace, PlayerTargetedOffer, room-ad purchase, club months); none of them have the same shape, so no further changes needed. - SEC-02: add Vortex:Authentication:TicketSingleUse (default false, preserving current behavior) and an optional TicketAbsoluteLifetimeSeconds cap. With TicketSingleUse on, a non-locked SSO ticket is deleted on first successful use instead of having its expiry pushed forward, closing the "observed once, reusable forever" replay window. Locked (persistent/CMS) tickets are intentionally left untouched either way. All 564 existing tests pass; build stays clean at 0 warnings.
TEST-01: these three subsystems had zero tests despite being the highest-consequence code to regress silently (a weakened cipher or a broken dispatch engine breaks everything downstream without a single existing test noticing). - Vortex.Crypto.Tests (34 tests): RC4 round-trip plus a classical known-answer vector, key-length guards, the CODE-02 argument validation on ProcessBytes, Peek non-mutation; RSA encrypt/decrypt round trip, sign/verify, corrupted-ciphertext rejection; DiffieService's real Diffie-Hellman exchange cross-checked against a manual ModPow, plus rejection of small-subgroup/out-of-range client public keys (0, 1, prime-1, prime). - Vortex.Authentication.Tests (13 tests, EF Core InMemory VortexDbContext): valid-ticket resolution, PlayerLoggedInEvent/ PlayerLoginFailedEvent publishing, expired-ticket deletion vs. the IsLocked exemption, the ExpiresAt ?? CreatedAt+TTL fallback, and the sliding-expiry refresh (skipped when locked). - Vortex.Pipeline.Tests (13 tests): drives EnvelopeHost directly with local test envelope/context types - handler dispatch, parallel vs. sequential execution, [Order]-attributed behavior sequencing, disposal-based unregistration, EnableInheritanceDispatch, the OnHandlerActivationError/OnHandlerInvokeError/OnNoHandlerRegistered callbacks, and MaxHandlerDegreeOfParallelism actually bounding concurrency. 624 total tests pass (564 pre-existing + 60 new); build stays clean at 0 warnings.
Closes the cheapest DoS vector the audit flagged: nothing bounded how many packets a session could send per second, and the protocol's own FloodControlMessageComposer was modeled but never sent by anything. - Transport level: RateLimitBehavior is a global IMessageBehavior registered against IMessageEvent itself (EnableInheritanceDispatch applies it to every concrete message via one registration) with [Order(int.MinValue)] so it runs before any other behavior, handler, or the grain call resolving the message context. Backed by TokenBucketRateLimiter, a per-session token bucket (default 50/s sustained, 100 burst, both configurable under Vortex:Networking:RateLimit) that self-sweeps stale sessions so it doesn't leak memory across reconnects. A rate-limited packet is dropped silently and counted in the existing packets_dropped_total metric (reason "rate_limited") rather than closing the connection. Since Vortex.Messages isn't a plugin module and its assembly is never scanned by PluginBootstrapper, a small MessageBehaviorRegistrationService triggers that scan for it, registered ahead of VortexEmulator so it's active before the network listeners open. - Chat level: RoomChatSystem now enforces a per-player minimum gap between chat lines, keyed by the room's existing (previously unenforced) ChatSettingsSnapshot.FloodSensitivity, and sends FloodControlMessageComposer with the remaining wait instead of silently dropping the line. Thresholds are new tunable defaults under RoomConfig.ChatFloodIntervalSeconds (4/2/1s for Extra/Normal/ Minimal), not a tuned game-balance decision - adjust freely. All 624 tests pass; build stays clean at 0 warnings.
Previously UseLocalhostClustering()/AddMemoryGrainStorage() were the
only code path that existed: the startup guard correctly refused to
run unclustered outside Development, but pointed operators at a
"configure a persistent provider" instruction they could only satisfy
by editing source and recompiling.
- OrleansHostConfig gains ClusteringProvider ("localhost" default |
"adonet"), GrainStorageProvider ("memory" default | "adonet" - only
PubSubStore, per ORL-01), and Invariant (defaults to "MySqlConnector",
the driver this project already ships via
Pomelo.EntityFrameworkCore.MySql). Defaults are unchanged, so nothing
about today's behavior moves until an operator opts in.
- "adonet" mode reuses Vortex:Database:ConnectionString rather than a
second connection-string surface, and registers MySqlConnector's
DbProviderFactory under the configured invariant since Orleans's
ADO.NET providers resolve it by name. The startup guard now only
fires when clustering is unclustered *and* storage is in-memory,
since either alone no longer means "cannot survive a restart".
- Added Microsoft.Orleans.Clustering.AdoNet and
Microsoft.Orleans.Persistence.AdoNet (10.2.1, matching the existing
Orleans package set) plus an explicit MySqlConnector pin.
- Documented the one real prerequisite this code can't satisfy itself:
Orleans's official clustering/persistence SQL scripts
(https://aka.ms/orleans-sql-scripts) still need to be applied to the
database before "adonet" mode is used, the same deployment-time step
EF migrations already are.
- Extended OrleansHostConfigValidator to check the provider names and
that a connection string exists when adonet is selected; updated
existing tests for its new constructor dependency and added 3 more
covering the new validation branches.
All 627 tests pass; build stays clean at 0 warnings.
…ate (OPS-02) RequiredServiceGuard already computed IsDegraded/DegradedServices for the console/logs, but nothing exposed it over HTTP, and there was no liveness/readiness probe for a container orchestrator to poll. - New GET /health on Vortex.WebApi: pings the database via IDbContextFactory<VortexDbContext>.Database.CanConnectAsync and reports RequiredServiceGuard's degraded state, returning Healthy/Degraded (200) or Unhealthy (503) with the specific list of degraded services in the body. - Forwarded RequiredServiceGuard and IDbContextFactory<VortexDbContext> into the WebApi's isolated DI container (WebApiWebHost already held a reference to the guard for startup reporting, just hadn't shared it with the endpoints). - Extended WebApiTestFactory with an in-memory DbContextFactory and a real RequiredServiceGuard so the new endpoint is exercised by an actual integration test rather than left uncovered. 628 tests pass (627 + 1 new); build stays clean at 0 warnings.
…ARCH-02, partial)
The audit's ARCH-02 R5 recommendation was to pull Trading/Moderation/
MysteryBox out into three separate Orleans grains, characterizing them
as low-coupling ("the trade has its own lifecycle", "isolated
reservation state"). Investigating the actual code before touching it
found the opposite: kick/ban mutate live avatar state
(AvatarModule.RemoveAvatarFromPlayerAsync), mute status is read
in-process on every chat message by RoomChatSystem, and mystery-box
sessions are torn down synchronously from the room tick, from an
avatar leaving, and from item removal - all of which currently get
single-grain-turn atomicity for free. Moving these to separate grains
would turn each of those into a cross-grain call with no transaction
to back it, in code that grants prizes and enforces moderation.
Given that risk, and no way to validate cross-grain consistency
behavior without a live multi-silo cluster, this applies the safer
half of the same idea instead: Moderation and MysteryBox move out of
RoomGrain's own partial-class files into RoomModerationSystem and
RoomMysteryBoxSystem, owned classes constructed the same way this
codebase already does for RoomChatSystem/RoomWiredSystem. RoomGrain
keeps every existing public/internal method as a one-line delegate, so
every external call site (PacketHandlers, RoomObjectModule, the wired
kick action, the tick loop, avatar-leave handling) is unchanged. This
is a pure move - no method's logic changed - and it's the same
pattern already proven safe by Chat/Wired, not a new design.
This reduces RoomGrain's own file size/complexity and gives these two
areas an isolated home to grow in, without gambling on unverified
cross-grain consistency for prize-granting and moderation-enforcement
code. Trading is the next candidate (largest of the three); full
separate-grain extraction remains a legitimate follow-up if someone
invests in a state-sync design and integration tests against a real
cluster first.
All 251 Vortex.Rooms.Tests pass; full solution build stays clean at 0
warnings.
…the R5 partial) Same pattern and same reasoning as the moderation/mystery-box move: trading needs the room's live avatar/object-id mapping (_state.AvatarsByPlayerId) on every step and is torn down synchronously from RoomGrain.Avatar.cs when a participant leaves, so it stays on the room grain rather than becoming a separate one - but moves out of RoomGrain's own partial-class file into RoomTradingSystem, an owned class built the same way as RoomChatSystem/RoomWiredSystem/ RoomModerationSystem/RoomMysteryBoxSystem. RoomGrain keeps every existing public/internal method as a one-line delegate, so no external call site changes. Pure move, no logic changed. This completes the ARCH-02 R5 work for this session: RoomGrain.cs itself now owns five focused systems (Chat, Wired, Moderation, MysteryBox, Trading) instead of carrying their bodies directly, matching an established pattern rather than inventing a new one. Actual separate-grain extraction - the audit's original ask, and the part that would shrink IRoomGrain's 109-method interface - remains a follow-up: it requires a cross-grain state-sync design and integration tests against a live multi-silo cluster that this session couldn't validate, for code that grants prizes and enforces moderation. All 628 tests pass (251 in Vortex.Rooms.Tests); build stays clean at 0 warnings.
…se (REV-01/R3) Revision20260701.cs was 3612 lines: two giant Dictionary initializers (header->parser, composer type->serializer) organized only by #region comments. Every new message meant editing that one file, making it a permanent merge-conflict magnet, and there was no way for a future revision to inherit from this one and override just a handful of entries - adding a second revision meant duplicating the whole file. - IRevisionMap/IRevisionMapBuilder (new, in Vortex.Primitives): a map registers its header/parser and type/serializer pairs into a builder; RevisionMapBuilder collects them and now throws on a duplicate header or composer type instead of silently overwriting it, so two maps accidentally claiming the same entry fails loudly the moment it happens instead of losing a registration silently. - RevisionBase: assembles a revision's Parsers/Serializers tables from a list of IRevisionMap instances, with virtual ConfigureParsers/ ConfigureSerializers hooks a descendant revision can override to add or replace specific entries without duplicating the rest. - Revision20260701.cs is now 59 lines: it lists 43 domain map instances and its revision id string. The three parsers needing ProtocolLimitsConfig (RemoveFriendMessageParser, AddItemsToTradeMessageParser, SaveRoomSettingsMessageParser) are each in their own domain map, taking the config as a constructor argument. - IRevision.Parsers/Serializers are now IReadOnlyDictionary instead of IDictionary (REV-03) - nothing outside revision construction should be able to mutate a revision's tables. - No parser/serializer logic changed - this only reorganizes which file registers which existing implementation against which existing header/type. Verified mechanically: the original two dictionaries had exactly 500 parser and 367 serializer entries; the 43 map files register exactly the same counts, confirmed again at runtime via Revision20260701.Parsers.Count/Serializers.Count. Combined with the earlier by-contract revision registration (RevisionRegistrationService, IRevisionManager.SetDefault), adding a message now touches one domain map file instead of a 3612-line one, and a second revision can inherit from this one instead of starting from a duplicated 4750-line copy. All 628 tests pass; build stays clean at 0 warnings.
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
2 participants
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.
before attempting it, so a cancelled purchase can no longer leave a
player permanently debited without their item.
transaction sites (PlayerWalletGrain, GroupDirectoryGrain,
RoomGrain.Trading) to run inside a retryable execution strategy with
a fresh DbContext per attempt.
optional MySqlServerVersion so ServerVersion.Parse can replace
AutoDetect (which opens a connection during DI configuration).
PlayerGrain.SetFigureAsync.
path or Debug-level logging as the cross-platform/production default.
(PackageHandler, Rc4Engine); fix two lingering nullability warnings
(RentableSpaceGrain, DashboardEndpoints) so the build is warning-free.
are now required dependencies wired through both network transports
instead of silently-null optionals; add a Vortex.packet.dropped metric
so packets that vanish during encoding are now visible.
revision instead of failing silently.
queue behind the 50ms room tick.
line to structured logging.
All 564 existing tests pass; build is clean at 0 warnings.