Repository navigation
Conversation
status's trust line describes only the server connected right now, so a script could not tell a player that has never been paired from one whose paired server is simply not connected yet. The installers need exactly that to offer pairing on a fresh install without nagging on an upgrade. 'paired servers: N' counts the stored pairing records, connected or not.
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
pairing_record_count()iterates all record slots and (viablob()) may allocate/parse each slot… · New
What changed in this PR
Adds a paired servers: N line to sendspin-cli status so scripts/installers can distinguish “never paired” from “paired but currently disconnected”.
Changes:
- Introduces
StateStore::pairing_record_count()and redefineshas_pairing_record()in terms of it. - Extends status snapshot/formatting to always report the number of stored pairing records.
- Updates docs and adds tests covering slot counting behavior and status output.
| File | Description |
|---|---|
| tests/state_store_test.cpp | Adds a unit test validating pairing record slot counting across freed/loaded states. |
| tests/control_test.cpp | Adds a formatting test ensuring paired servers appears even when disconnected. |
| src/state_store.h | Declares pairing_record_count() API. |
| src/state_store.cpp | Implements record-slot counting and updates has_pairing_record() accordingly. |
| src/main.cpp | Injects StateStore into ControlDispatcher to populate paired_servers in status snapshots. |
| src/control_common.cpp | Prints paired servers in status output. |
| src/control.h | Adds paired_servers field to StatusSnapshot. |
| docs/wiki/Pairing-a-Player.md | Updates examples/grep and explains the distinction between trust and paired servers. |
| docs/wiki/Controlling-the-Player.md | Updates status output example to include paired servers. |
Review effort: Lite
Comment on lines
+18
to
+19
| `trust` is about the server connected right now; `paired servers` counts every server this | ||
| player has paired with, connected or not. |
Comment on lines
497
to
+500
| snapshot.client_id = this->client_.client_id(); | ||
| snapshot.connected = this->client_.is_connected(); | ||
| snapshot.trust = this->pairing_listener_.trust(); | ||
| snapshot.paired_servers = this->state_store_.pairing_record_count(); |
| const std::optional<std::vector<uint8_t>> record = | ||
| this->blob(sendspin::persistence_keys::record_slot_key(slot)); | ||
| // A freed slot is kept as zeroes. | ||
| if (record.has_value() && |
| std::optional<std::vector<uint8_t>> blob(const std::string& key) const; | ||
| bool set_blob(const std::string& key, const uint8_t* data, size_t len); | ||
|
|
||
| /// How many servers the library has a pairing record stored for. |
A slot of the wrong size, or with an all-zero PSK, was counted as a pairing because it held a non-zero byte. Ask decode_pairing_record(), the library's own rule, instead.
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.
Why
status'strust:line describes only the server connected right now. While nothing is connected it readsnot connected, so a script cannot tell a player that has never been paired from one whose paired server just hasn't connected yet.The guided installers are about to offer pairing during setup: show the pairing token, and display a dynamic code live while the server pairs. They need exactly that distinction, so they can offer pairing on a fresh install and stay quiet on an upgrade. That is the same "an upgrade asks nothing" rule they already follow for the name and output.
What
statusprintspaired servers: N, the number of servers with a stored pairing record, whether or not one is connected. It sits right aftertrust:.StateStore::pairing_record_count()counts the occupied record slots, using the same rulehas_pairing_record()used: a slot the library freed is kept as zeroes and doesn't count.has_pairing_record()is now defined ascount > 0, so its one caller (whether to log the pairing token at startup) is unchanged.ControlDispatchertakes aconst StateStore&. It is read on the main loop only, which matches the store's threading rule.statusexamples in Controlling the Player and Pairing a Player. The latter'sgrep -E '^(trust|pairing)'wouldn't match the new line, so it now also matchespaired.The library has no query for its pairing records. It persists every record through our persistence provider into the
StateStore, which makes the store the place to count them.Testing
StateStore.CountsEachOccupiedPairingSlot: occupied slots count, freed slots don't, the count survives a reload, and a slot zeroed on unpairing drops out. NewFormatStatus.PairedServersAreReportedEvenWhileDisconnected.-Werror): 519/519 passed.trust: not connected/paired servers: 1. A fresh state directory reportspaired servers: 0. The copy was deleted afterwards.scripts/format.sh --checkwith clang-format 20.1.8.