fix(velvet): treat an unknown race as unavailable instead of allowed - #61
fix(velvet): treat an unknown race as unavailable instead of allowed#61Valiice wants to merge 1 commit into
Conversation
d93139f to
6a2c639
Compare
There was a problem hiding this comment.
Builds clean in Release. Comments inline.
Branch does not merge: based on 8bf1669f, 222 commits behind. Watch the ImGuiHelpers.GlobalScale to UiScale.Current migration on rebase, CI fails on it.
Your open questions: presence stopping at the title screen is correct, leave it. The stored lalafell flag may have bad rows from master, I will check Aethernet.
|
|
||
| private volatile byte race = UnknownRaceId; | ||
|
|
||
| public byte? Race => race == UnknownRaceId ? null : race; |
There was a problem hiding this comment.
Two volatile loads. If Forget() lands between them this returns a non-null (byte?)0, so IsLalafell answers false for an unknown race. Verified against real Dalamud: 2.2M bad reads in 3s under contention.
| public byte? Race => race == UnknownRaceId ? null : race; | |
| public byte? Race | |
| { | |
| get | |
| { | |
| var observed = race; | |
| return observed == UnknownRaceId ? null : observed; | |
| } | |
| } |
| } | ||
|
|
||
| if (IsLalafellCharacter() || store.AccessBlocked) | ||
| if (Unavailable) |
There was a problem hiding this comment.
velvet.unavailableBody reads "not available on Lalafell characters". Unknown lands here too now, so logging out to character select tells a Highlander they are a Lalafell. Needs its own empty state: new LocString plus all nine JSONs.
| internal sealed partial class VelvetShell : IPhoneApp | ||
| { | ||
| private const float HeartbeatSeconds = 45f; | ||
| private const long RaceProbeIntervalMilliseconds = 1000; |
There was a problem hiding this comment.
This one second floor means every login with Velvet open shows that screen briefly, not just the title screen. Skip the rate limit while the race is unknown.
| configuration.VelvetAcknowledgedGate && | ||
| configuration.VelvetAcknowledgedGateVersion >= Configuration.VelvetGateVersion; | ||
|
|
||
| private bool Unavailable => race.IsLalafell is not false || store.AccessBlocked; |
There was a problem hiding this comment.
Nit: is not false is a double negative. IsLalafellOrUnknown on RaceWatch would read better here.
What
Velvet is meant to be unavailable on Lalafell characters. The check behind that can't tell your race until a character is loaded, and when it can't tell it answers "not a Lalafell". So at the login screen a Lalafell gets in. This gives the check a third answer, "don't know yet", and treats that as unavailable.
Why
Came out of this report: https://discord.com/channels/1527213298087493732/1530436723899633855
cachedLalafellis a plain bool defaulting to false and both early returns inIsLalafellCharacter()hand it back, so "couldn't read the race" and "confirmed not a Lalafell" were the same value. With no character loaded it is always false.Two things consume it:
&lalafell=false.sinceHeartbeatstarts already expired, so the first drawn frame sends it before any read could have succeeded.The 403 doesn't cover this on its own.
EnsureMereturns early onceme is not null, andmeandaccessBlockedonly reset on an account change, so the 403 can only land on the first profile load after signing in. After one 200 the client side check is the only thing left. The gate at the top ofDrawalso runs before the profile load it depends on has finished.How to test
Step 1 is the one that matters.
Extra
RaceWatch(new,Core/Game/) holds the race id it read, or null if it doesn't have one yet, andIsLalafellcomes off that. Race id 0 is ignored since it isn't a real race, and a failed read never overwrites an answer it already has. It's a volatile byte becausebool?is two fields and can be caught half written from the draw thread.VelvetShellreads it on aFrameworkTicker(1s, gated on the app being installed) rather than duringDraw, so it no longer depends on whetherDrawcounts as the framework thread. It forgets the answer only on a real logout, so a loading screen or a zone change keeps the last known race.The two call sites want opposite defaults, which is the reason for the tri-state: the gate blocks on unknown, the heartbeat sends nothing on unknown instead of a guess.
11 tests in
RaceWatchTests.Couple of open questions:
lalafellflag we send? If it does, some stored tags are probably wrong, since master can send false for a character it never read.&lalafell=is always appended, so it needs a server side change.Checklist
dotnet build -c ReleasepassesWindows/Components/widgets, nowhatcomments