fix: hold the character-select step with the server, not a client hook

Stan's review on the select-screen hold is that it re-adds a deleted client
hook and should be a server implementation instead. The hook is deleted here;
the hold is carried by what the server publishes.

The client was walking off the screen because the server invented a pick the
player never made:

- create_character marked the new character selected and cleared every other
  flag, and delete_character handed the selection to a survivor whenever the
  deleted slot held it. Moving the account onto a character by itself is what
  the real pick (opcodes 504/505) does, so the step had nothing left to wait
  for. Both now change the roster without selecting anything; only the player
  picks.
- The opcode 501/502 handlers staged a queuez::SelectCharacter Family-4 move to
  publish that invented pick. They now arm the account resync they already arm
  and name the revision it publishes (queuezState.family4Version + 1) -- the
  same "name the following resync" contract the claim path below them uses --
  so the reply names a publication that actually arrives instead of one that
  moves the selection.

Two account-resync companions treated "nothing selected" as a failure. That is
now a real, expected state (it is what the select screen is), so the Family-0
appearance and Family-3 roster lanes return without publishing instead of
failing, which also stops the ability-refresh retry lane from re-running
against a screen that is waiting for its own pick. The Family-4 account frame
still goes out, so the roster count moves immediately.

Removes Sunrise/src/client/hooks/bootflow/character_select_hold.cpp, its
lifecycle registration, its declarations, and its project entry.

Sunrise/Sunrise.vcxproj

@@ -322,7 +322,6 @@
     </ClCompile>
     <ClCompile Include="src\client\hooks\bootflow\bootflow_hook_lifecycle.cpp" />
     <ClCompile Include="src\client\hooks\bootflow\bootflow_texture_override.cpp" />
-    <ClCompile Include="src\client\hooks\bootflow\character_select_hold.cpp" />
     <ClCompile Include="src\client\hooks\bootflow\lifetime_gate_probe.cpp" />
     <ClCompile Include="src\client\hooks\bootflow\world_step.cpp" />
     <ClCompile Include="src\client\hooks\bootflow\spawn\slice_set_sample.cpp" />

Sunrise/src/client/hooks/bootflow/bootflow_hook_lifecycle.cpp

@@ -20,15 +20,12 @@ bool install() noexcept {
     const bool worldStep = install_world_step();
     const bool sliceSet = spawn::install_targets();
     const bool probe = install_lifetime_gate_probe();
-    const bool characterSelectHold = install_character_select_hold();
-    g_installed.store(worldStep || sliceSet || probe || characterSelectHold,
-                      std::memory_order_release);
-    return worldStep && sliceSet && probe && characterSelectHold;
+    g_installed.store(worldStep || sliceSet || probe, std::memory_order_release);
+    return worldStep && sliceSet && probe;
 }
 
 /** Detaches the probe and clears both accessors, in the reverse order of install. */
 void uninstall() noexcept {
-    uninstall_character_select_hold();
     uninstall_lifetime_gate_probe();
     spawn::uninstall_targets();
     uninstall_world_step();

Sunrise/src/client/hooks/bootflow/character_select_hold.cpp

@@ -1,108 +0,0 @@
-#include <array>
-#include <atomic>
-#include <cstddef>
-#include <cstdint>
-#include <cstring>
-#include <string_view>
-
-#include "../../../core/logging/log.h"
-#include "../../hooking/detour.h"
-#include "internal.h"
-
-namespace sunrise::client::hooks::bootflow {
-namespace {
-
-/**
- * The character sign-in step's enter handler, anchored on its two setup stores.
- * Those write the step's latch field and the paired setup words, so no sibling step matches.
- */
-constexpr std::string_view kEnterSignatureText =
-    "40 53 48 83 EC ? 48 8B D9 C7 41 38 FF FF FF FF 66 C7 41 3C 00 00 33 D2";
-/** Compiled pattern bytes of the signature text above. */
-constexpr auto kEnterSignature =
-    signature<signature_length(kEnterSignatureText)>(kEnterSignatureText);
-
-/** Fields of the boot step this hook touches, as byte offsets from the step's own base. */
-struct StepLayout {
-    /**
-     * The stay flag. The exit gate needs it non-zero to stay on the select screen. The enter
-     * handler clears it, so the step would skip selection on its first frame.
-     */
-    static constexpr std::size_t stayFlag = 72;
-};
-
-/** Value the step's own listener writes to hold the screen. Matched here. */
-constexpr std::uint8_t kHold = 1;
-
-using EnterHandler = void(__fastcall*)(std::byte*);
-
-hooking::detour::Handle g_handle{};
-std::atomic<EnterHandler> g_original{nullptr};
-std::atomic_bool g_reported{false};
-
-/**
- * Holds the character-select screen after the step sets itself up.
- * Runs once per boot on the boot-step thread, after the original writes its defaults, so the
- * original cannot overwrite this write.
- * @param step Borrowed boot-step base pointer.
- */
-__declspec(noinline) void __fastcall enter_handler(std::byte* step) noexcept {
-    const EnterHandler original = g_original.load(std::memory_order_acquire);
-    if (original != nullptr) {
-        original(step);
-    }
-    if (step == nullptr) {
-        return;
-    }
-    std::memcpy(step + StepLayout::stayFlag, &kHold, sizeof kHold);
-    if (!g_reported.exchange(true, std::memory_order_relaxed)) {
-        core::log::write(core::log::Channel::client,
-                         core::log::Level::info,
-                         "ev=bootflow stage=character_select result=held");
-    }
-}
-
-} // namespace
-
-/**
- * Attaches the character-select hold: without it, the sign-in boot step clears its own stay
- * flag on entry and the screen falls straight through to orbit, whatever character was picked,
- * created, or removed. The client's exit gate reads this flag every frame and only leaves the
- * screen once something (the player picking a character) sets it again.
- * @return True when the target is found and the detour attaches.
- */
-bool install_character_select_hold() noexcept {
-    if (g_handle.attached) {
-        return true;
-    }
-    std::byte* const target = scan_main_image_unique(kEnterSignature, "character_signin_enter");
-    if (target == nullptr) {
-        core::log::write(core::log::Channel::client,
-                         core::log::Level::warn,
-                         "ev=bootflow stage=character_select result=fail reason=target");
-        return false;
-    }
-    const hooking::detour::Spec spec{target, reinterpret_cast<void*>(&enter_handler)};
-    if (!hooking::detour::install(spec, g_handle)) {
-        core::log::write(core::log::Channel::client,
-                         core::log::Level::warn,
-                         "ev=bootflow stage=character_select result=fail reason=attach");
-        return false;
-    }
-    g_original.store(reinterpret_cast<EnterHandler>(g_handle.original), std::memory_order_release);
-    core::log::write(core::log::Channel::client,
-                     core::log::Level::info,
-                     "ev=bootflow stage=character_select result=ok");
-    return true;
-}
-
-/** Detaches the character-select hold. */
-void uninstall_character_select_hold() noexcept {
-    if (g_handle.attached) {
-        (void)hooking::detour::uninstall(g_handle);
-    }
-    g_original.store(nullptr, std::memory_order_release);
-    g_reported.store(false, std::memory_order_release);
-}
-
-} // namespace sunrise::client::hooks::bootflow

Sunrise/src/client/hooks/bootflow/internal.h

@@ -28,14 +28,4 @@ void uninstall_world_step() noexcept;
 /** Detaches the lifetime gate probe. */
 void uninstall_lifetime_gate_probe() noexcept;
 
-/**
- * Attaches the character-select hold, so the sign-in boot step stays on the character-select
- * screen instead of falling through to orbit on its own.
- * @return True when the target is found and the detour attaches.
- */
-[[nodiscard]] bool install_character_select_hold() noexcept;
-
-/** Detaches the character-select hold. */
-void uninstall_character_select_hold() noexcept;
-
 } // namespace sunrise::client::hooks::bootflow

Sunrise/src/middleware/web_service/messages/opcode501_codec.cpp

@@ -21,11 +21,7 @@ bool encode_response(const Message& message,
     if (output.size() < kEnvelopeHeaderSize) {
         return false;
     }
-    encoding::write_u16_be(output.first<encoding::kU16Size>(), message.opcode);
-    encoding::write_u32_be(output.subspan<encoding::kU16Size, encoding::kU32Size>(),
-                           message.transactionId);
-
-    encoding::bits::Writer writer(output.subspan(kEnvelopeHeaderSize));
+    encoding::bits::Writer writer = begin_response(message, output);
     // The id carries no presence bit or bias.
     bool encoded = status::write_fields(writer, ResponseShape::statusPair, status)
                    && writer.write(characterSoid, kCharacterSoidWidth)

Sunrise/src/server/bap/encrypted/body/bap_service_body.cpp

@@ -238,23 +238,19 @@ bool process(const ServiceRoute& route,
             // outcome flag at the end of this file only lands on the handled path. Without this
             // the character is gone from disk while the client keeps a roster that names it.
             sunrise::server::bap::arm_account_resync_everywhere();
-            // The roster shrank and delete_character has already moved the selection onto a
-            // survivor, so the client needs the Family-4 character object moved with it. Opcode
-            // 502 feeds the Client's Family-4 version wait: the reply has to name the revision the
-            // wait should expect, the staged one when a publication is coming and the no-wait
-            // constant when none is -- left at its default the field reads as revision zero, which
-            // the wait blocks on, and the roster screen never finishes the delete it just asked for.
+            // The roster shrank, and the client is still on the character-select step. Nothing
+            // here may move the selection: a selected character is a pick, and a pick is what
+            // leaves the screen for orbit, so the delete would skip the roster it just changed.
+            // The account object the armed resync re-encodes already carries the new roster count,
+            // so the reply only has to name the revision that resync will publish -- the same
+            // "name the following resync" contract the claim path below uses -- and the step waits
+            // on it instead of walking on. Left at its default the field reads as revision zero,
+            // which the wait blocks on, and the roster screen never finishes the delete it asked
+            // for.
             std::int32_t deletionVersion = middleware::web_service::kNoFamily4Publication;
-            const std::uint64_t survivorSoid =
-                state::account::selected_character_soid(state::account_snapshot());
-            auto* deletionSelect = emplace_transaction<queuez::SelectCharacter>(outcome);
-            if (survivorSoid != 0 && deletionSelect != nullptr
-                && queuez::stage_select_character(queuezState, survivorSoid, *deletionSelect)) {
-                outcome.hasSelectCharacter = true;
-                deletionVersion = deletionSelect->after.family4Version;
-            } else {
-                clear_transaction(outcome);
-                sunrise::server::bap::arm_account_resync_everywhere();
+            if (queuezState.family4Active
+                && queuezState.family4Version != (std::numeric_limits<std::int32_t>::max)()) {
+                deletionVersion = queuezState.family4Version + 1;
             }
             core::log::write(
                 core::log::Channel::server, core::log::Level::info, "ev=ws502 stage=delete result=ok");
@@ -292,20 +288,18 @@ bool process(const ServiceRoute& route,
             // As in the delete branch: the account is already changed and written, but the caller
             // can still drop the frame, and the outcome flag only lands on the handled path.
             sunrise::server::bap::arm_account_resync_everywhere();
-            // create_character marks the new character selected, and the client is waiting for the
-            // Family-4 character object to move onto it. A change-character patch cannot do that
-            // here: it refuses unless family three is in its normal phase, and the creation flow
-            // leaves it outside that phase, so the client would wait forever on a publication that
-            // was never staged. Selecting is both ungated and what the client actually asked for.
+            // The new character joins the roster unselected (see create_character), and the client
+            // is still on the character-select step. Selecting it here would be a pick the player
+            // never made, and a pick is what leaves the step for orbit, so creation would skip the
+            // roster it just changed. The armed resync re-encodes the account object with the new
+            // roster count, so the reply only names the revision that resync will publish and the
+            // step waits on it. A change-character patch cannot do this: it refuses unless family
+            // three is in its normal phase, and the creation flow leaves it outside that phase, so
+            // the client would wait forever on a publication that was never staged.
             std::int32_t creationVersion = middleware::web_service::kNoFamily4Publication;
-            auto* creationSelect = emplace_transaction<queuez::SelectCharacter>(outcome);
-            if (creationSelect != nullptr
-                && queuez::stage_select_character(queuezState, characterSoid, *creationSelect)) {
-                outcome.hasSelectCharacter = true;
-                creationVersion = creationSelect->after.family4Version;
-            } else {
-                clear_transaction(outcome);
-                sunrise::server::bap::arm_account_resync_everywhere();
+            if (queuezState.family4Active
+                && queuezState.family4Version != (std::numeric_limits<std::int32_t>::max)()) {
+                creationVersion = queuezState.family4Version + 1;
             }
             core::log::write(core::log::Channel::server,
                              core::log::Level::info,

Sunrise/src/server/bap/encrypted/push/queuez/queuez_banner_push.cpp

@@ -246,7 +246,11 @@ bool append_account_resync_appearance_notification(
     const state::AccountState account = state::account_snapshot();
     const std::uint64_t selected = state::account::selected_character_soid(account);
     if (selected == 0) {
-        return false;
+        // The appearance record belongs to the character the player picked. Before a pick there is
+        // none, and the account change that armed this resync already went out in the Family-4 frame
+        // ahead of it. Refusing would report a failure that frame covered and leave the retry lane
+        // re-running on a screen whose whole purpose is to wait for that pick.
+        return true;
     }
     if (before.family0Character != selected) {
         return append_banner_move_notification(
@@ -288,6 +292,14 @@ bool append_account_resync_roster_notification(Scratch& scratch,
     }
     const state::AccountState account = state::account_snapshot();
     const std::uint64_t selected = state::account::selected_character_soid(account);
+    if (selected == 0) {
+        // The character record and account roster this lane carries describe the picked character.
+        // Before a pick there is none to describe, and the account change that armed this resync
+        // already went out in the Family-4 frame ahead of it. Refusing would report a failure that
+        // frame covered and leave the retry lane re-running on a screen that waits for the pick
+        // which would end it.
+        return true;
+    }
     std::size_t characterIndex = account.characterCount;
     for (std::size_t index = 0; index < account.characterCount; ++index) {
         if (account.characters[index].soid == selected) {
@@ -297,7 +309,7 @@ bool append_account_resync_roster_notification(Scratch& scratch,
     }
     queuez::RosterAppearanceRefresh refresh{};
     snapshot::Prepared prepared{};
-    if (selected == 0 || characterIndex >= account.characterCount
+    if (characterIndex >= account.characterCount
         || !queuez::stage_roster_appearance_refresh(before, selected, includeRoster, refresh)
         || !snapshot::prepare_roster_appearance_refresh(
             scratch, refresh, account.characters[characterIndex], characterIndex, prepared)

Sunrise/src/state/runtime/state_account_roster_runtime.cpp

@@ -216,11 +216,9 @@ bool create_character(std::uint8_t characterClass,
     character.appearanceHeader = appearanceHeader;
     character.appearanceHeaderValid = true;
 
-    // Select the new character and deselect all others.
-    for (std::size_t i = 0; i < candidate.characterCount; ++i) {
-        candidate.characters[i].selected = false;
-    }
-    character.selected = true;
+    // The new character joins the roster unselected. Marking it selected here would be a pick
+    // the player never made, and a pick is what leaves the character-select step for orbit, so
+    // creation would skip the roster it just changed. Nothing is picked until the player picks.
     ++candidate.characterCount;
 
     // Counted above so the fresh-SOID scan inside sees this character's own items as they are
@@ -289,17 +287,10 @@ bool delete_character(std::uint64_t characterSoid) noexcept {
     --candidate.characterCount;
     candidate.characters[candidate.characterCount] = {};
 
-    // Select the first character if the deleted one was selected.
-    bool hasSelection = false;
-    for (std::size_t i = 0; i < candidate.characterCount; ++i) {
-        if (candidate.characters[i].selected) {
-            hasSelection = true;
-            break;
-        }
-    }
-    if (!hasSelection && candidate.characterCount > 0) {
-        candidate.characters[0].selected = true;
-    }
+    // Removing a character never creates a selection. The deleted character's own flag left with
+    // its slot, and handing the selection to a survivor would be a pick the player never made --
+    // which is what leaves the character-select step for orbit, skipping the shrunken roster the
+    // delete just produced. The selection moves only when the player picks (opcodes 504 and 505).
 
     if (!account::valid(candidate) || !investment::store::write_account(candidate)) {
         investment::store::g_mutex.unlock();