Desync auto-resync — host sends state on desync event #6

New Issue

pbwraith · 2026-08-25T06:03:46+02:00

pbwraith commented

2026-08-25 06:03:46 +02:00

Repo: gopher64-netplay (fork of https://github.com/gopher64/gopher64)
Context: Full scope doc at upstream analysis — see commit history. Netplay uses GGRS over WebRTC (matchbox_socket), reliable channel carries NetplayMessage { name, data } in 16KB chunks. Savestates serialize full Device via postcard + compress.

Goal: When GGRS fires DesyncDetected, the HOST snapshots state and ships it to the desynced peer.

Background:

poll_clients in src/netplay.rs handles GgrsEvent::DesyncDetected { .. } — currently only prints + onscreen message (lines 321-327)
GGRS DesyncDetection::On { interval: 60 } is already configured
Downstream of #1 (message types), #2 (in-memory state), #5 (frame reset)

Acceptance:

In poll_clients DesyncDetected handler:
- Host (player_number==0) creates create_savestate_in_memory
- Host pauses frame loop (enter a "resyncing" state: advance_frame returns immediately, like the disconnected path)
- Host sends resync_state message (name from #1) to all peers
- Host waits for resync_ready acks, then calls frame reset (#5) and resumes
Non-host: ignore the event (state arrives as message)
Onscreen: "Desync detected — resyncing…" then "Resynced"
If resync completes within 5s with no errors, no session interruption; timeout → drop to the existing disconnect behavior
Tests: inject synthetic DesyncDetected event, assert host sends resync_state and pauses loop; assert non-host ignores.

Verification: cargo test; manual desync test with latency injection if available in test setup.

**Repo:** gopher64-netplay (fork of https://github.com/gopher64/gopher64) **Context:** Full scope doc at upstream analysis — see commit history. Netplay uses GGRS over WebRTC (matchbox_socket), reliable channel carries `NetplayMessage { name, data }` in 16KB chunks. Savestates serialize full Device via postcard + compress. **Goal:** When GGRS fires `DesyncDetected`, the HOST snapshots state and ships it to the desynced peer. **Background:** - `poll_clients` in `src/netplay.rs` handles `GgrsEvent::DesyncDetected { .. }` — currently only prints + onscreen message (lines 321-327) - GGRS `DesyncDetection::On { interval: 60 }` is already configured - Downstream of #1 (message types), #2 (in-memory state), #5 (frame reset) **Acceptance:** - In `poll_clients` `DesyncDetected` handler: - Host (player_number==0) creates `create_savestate_in_memory` - Host pauses frame loop (enter a "resyncing" state: `advance_frame` returns immediately, like the `disconnected` path) - Host sends `resync_state` message (name from #1) to all peers - Host waits for `resync_ready` acks, then calls frame reset (#5) and resumes - Non-host: ignore the event (state arrives as message) - Onscreen: "Desync detected — resyncing…" then "Resynced" - If resync completes within 5s with no errors, no session interruption; timeout → drop to the existing disconnect behavior - Tests: inject synthetic `DesyncDetected` event, assert host sends `resync_state` and pauses loop; assert non-host ignores. **Verification:** `cargo test`; manual desync test with latency injection if available in test setup.

pbwraith referenced this issue

2026-08-25 06:03:46 +02:00

Desync auto-resync — client receives and loads state #7

pbwraith added the

netplay

p2

rust

labels 2026-08-25 06:04:35 +02:00

pbwraith added the

in-progress

label 2026-08-25 07:40:10 +02:00

pbwraith referenced this issue from a commit

2026-08-25 07:44:45 +02:00

Desync auto-resync: host sends state on desync event (#6)

pbwraith referenced a pull request that will close this issue

2026-08-25 07:45:01 +02:00

Desync auto-resync: host sends state on desync event (#6) #10

pbwraith added

in-review

and removed

in-progress

labels 2026-08-25 07:45:17 +02:00

pbwraith commented

2026-08-25 07:45:31 +02:00

/tmp/comment_6.md

pbwraith referenced this issue from a commit

2026-08-25 07:49:18 +02:00

Client receives and loads full state during desync resync (#7)

pbwraith referenced this issue

2026-08-25 07:49:31 +02:00

Desync auto-resync: client receives and loads full state (#7) #11

pbwraith referenced this issue

2026-08-26 13:27:23 +02:00

Desync auto-resync: host sends state on desync event (#6) #10

pbwraith commented

2026-08-26 13:57:18 +02:00

[orphaned-review repair] No standalone reviewable PR for #6.

PR #10 (head issue-6, "Desync auto-resync: host sends state on desync event (#6)") is CLOSED and UNMERGED — closed deliberately as SUBSUMED by the sweep review on 2026-08-26: the host-half work ships inside !11 (PR #11, issue-7, open, currently bounce-in-repair). Independently re-verified containment rather than trusting the comment:

git merge-base --is-ancestor origin/issue-6 origin/issue-7  ->  TRUE

commits on issue-6 not in main: 48598f4 (#6 host sends state) and eb5cf56 (#5 frame reset) — both carried by issue-7. The review record explicitly directs NOT to reopen/repair both branches ("close this one as subsumed rather than repairing both"), so a reopen would duplicate !11, and #10's head also conflicts with the post-!14 main (src/netplay.rs).

Therefore in-review is removed (there is no reviewable open PR for #6 itself) and needs-guidance added.

One question for the owner: close #6 as subsumed once !11 merges, or file a follow-up to verify #6's acceptance criteria (host snapshot + resync_state broadcast + 5s timeout) explicitly against the merged state before closing?

[orphaned-review repair] No standalone reviewable PR for #6. PR #10 (head `issue-6`, "Desync auto-resync: host sends state on desync event (#6)") is CLOSED and UNMERGED — closed deliberately as SUBSUMED by the sweep review on 2026-08-26: the host-half work ships inside !11 (PR #11, `issue-7`, open, currently bounce-in-repair). Independently re-verified containment rather than trusting the comment: git merge-base --is-ancestor origin/issue-6 origin/issue-7 -> TRUE commits on `issue-6` not in main: `48598f4` (#6 host sends state) and `eb5cf56` (#5 frame reset) — both carried by `issue-7`. The review record explicitly directs NOT to reopen/repair both branches ("close this one as subsumed rather than repairing both"), so a reopen would duplicate !11, and #10's head also conflicts with the post-!14 main (src/netplay.rs). Therefore `in-review` is removed (there is no reviewable open PR for #6 itself) and `needs-guidance` added. One question for the owner: close #6 as subsumed once !11 merges, or file a follow-up to verify #6's acceptance criteria (host snapshot + resync_state broadcast + 5s timeout) explicitly against the merged state before closing?

pbwraith added

needs-guidance

and removed

in-review

labels 2026-08-26 13:58:02 +02:00

pbwraith commented

2026-08-26 19:48:24 +02:00

Answering the open question, and clearing `needs-guidance`

The question left for the owner on 2026-08-26 was:

close #6 as subsumed once !11 merges, or file a follow-up to verify #6's
acceptance criteria (host snapshot + resync_state broadcast + 5s timeout)
explicitly against the merged state before closing?

Answer: verify against the merged state first, then close. Do not auto-close as
subsumed. This is a design/process call, not an owner risk decision, so it does
not need to wait on anyone.

Reasoning: containment is a claim about commits, not about acceptance.
I re-verified the containment itself this sweep —

compare issue-7...issue-6  ->  total_commits=0, ahead=0

so issue-6 is genuinely fully contained in issue-7 (!11). But !11 was
scoped and reviewed as #7, "client receives and loads full state". This
issue's acceptance list is the host half and contains four specific behaviours
that #7's review never had a reason to check:

host (player_number == 0) creates create_savestate_in_memory on DesyncDetected
host pauses the frame loop (advance_frame returns immediately, like the disconnected path)
host waits for resync_ready acks, then calls the #5 frame reset and resumes
5s timeout falls back to the existing disconnect behaviour; non-host ignores the event

Item 4 in particular is a failure-path behaviour that a client-side review would
not exercise. Closing #6 on containment alone would retire those criteria without
anyone having checked them — the same shape as a Closes #N retiring work that
was really someone else's.

What I changed and why

removed needs-guidance — the question is answered, and leaving the label
on means the answer can never be acted upon (the label excludes the issue from
the worker lane at gitea-worker-lane.py:143)
added blocked, prerequisite named: !11 must merge. There is nothing to
implement until then; the verification is the remaining work, and it needs a
merged tree to run against.

When !11 lands: check those four behaviours against merged main, record the
result here, then close. If any of them is missing, that is a new issue, not a
reopen of this one.

## Answering the open question, and clearing `needs-guidance` The question left for the owner on 2026-08-26 was: > close #6 as subsumed once !11 merges, or file a follow-up to verify #6's > acceptance criteria (host snapshot + resync_state broadcast + 5s timeout) > explicitly against the merged state before closing? **Answer: verify against the merged state first, then close. Do not auto-close as subsumed.** This is a design/process call, not an owner risk decision, so it does not need to wait on anyone. Reasoning: containment is a claim about *commits*, not about *acceptance*. I re-verified the containment itself this sweep — compare issue-7...issue-6 -> total_commits=0, ahead=0 so `issue-6` is genuinely fully contained in `issue-7` (`!11`). But `!11` was scoped and reviewed as **#7, "client receives and loads full state"**. This issue's acceptance list is the *host* half and contains four specific behaviours that #7's review never had a reason to check: 1. host (`player_number == 0`) creates `create_savestate_in_memory` on `DesyncDetected` 2. host pauses the frame loop (`advance_frame` returns immediately, like the disconnected path) 3. host waits for `resync_ready` acks, then calls the #5 frame reset and resumes 4. **5s timeout falls back to the existing disconnect behaviour**; non-host ignores the event Item 4 in particular is a failure-path behaviour that a client-side review would not exercise. Closing #6 on containment alone would retire those criteria without anyone having checked them — the same shape as a `Closes #N` retiring work that was really someone else's. ### What I changed and why - **removed `needs-guidance`** — the question is answered, and leaving the label on means the answer can never be acted upon (the label excludes the issue from the worker lane at `gitea-worker-lane.py:143`) - **added `blocked`, prerequisite named: `!11` must merge.** There is nothing to implement until then; the verification is the remaining work, and it needs a merged tree to run against. When `!11` lands: check those four behaviours against merged `main`, record the result here, then close. If any of them is missing, that is a new issue, not a reopen of this one.

pbwraith added

blocked

and removed

needs-guidance

labels 2026-08-26 19:48:25 +02:00

pbwraith added the

manual-only

label 2026-08-26 20:25:35 +02:00

pbwraith referenced this issue

2026-08-26 23:54:29 +02:00

Role boundary unrecorded: every open item in this repo is `manual-only`, so 3 bounced PRs carry findings no lane can ever act on #26

pbwraith closed this issue