fix(messaging): guard handle_send against re-entrancy to prevent dropped sends #211

Merged

rawdaGastan merged 1 commit from development_fix_send_reentrancy_race into development

2026-05-06 13:04:08 +00:00

rawdaGastan commented

2026-05-06 12:50:07 +00:00

Member

Summary

Rapid clicks on .send-btn (or repeated Enter) were spawning N concurrent send_message futures racing on the same client, with most resolutions silently dropped. The existing sending.set(true) ran inside the spawned future, so all clicks completed their synchronous closure body (clone + spawn) before any future was polled — N futures queued before the first one had a chance to flip sending. Button's in-handler click-guard could not rescue this because the prop value the guard reads is only updated on the next render, which itself only happens after the parent flips sending.

This PR adds a single synchronous re-entrancy guard at the top of handle_send and lifts sending.set(true) out of the spawned future so it runs synchronously, before spawn. The terminal sending.set(false) at the end of the future is unchanged and still covers both Ok and Err.

Closes #183

Changes

archipelagos/messaging/src/archipelago.rs: in the handle_send closure
- Added if *sending.peek() { return; } as the first statement (no subscription, synchronous read).
- Moved sending.set(true) from inside the spawned future to before spawn, inside the if let Route::Chat { sid, .. } branch (so we don't latch sending when the route isn't a chat).
- Existing sending.set(false) at the end of the future is unchanged.

 archipelagos/messaging/src/archipelago.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Test Results

cargo test -p hero_archipelagos_messaging --lib: 5/5 passed (the PR #210 tests still green).
cargo check -p hero_archipelagos_messaging: clean.
cargo check -p hero_archipelagos_messaging --target wasm32-unknown-unknown: clean.
cargo check --workspace --exclude hero_archipelagos_messaging: clean (9 pre-existing unused_mut warnings in server/src/main.rs are unrelated).

Live verified in browser: make run -> http://localhost:8886 -> messaging DM. Pasted the issue's 10-click DevTools snippet, observed exactly 1 message bubble appear, F5, re-opened the chat, exactly 1 of 10 rapid#N messages persisted (vs 0 before the fix). Send button reflected the disabled state during the in-flight RPC. Subsequent single send worked normally.

Test plan

Build: native + wasm32 clean.
PR #210 tests still pass.
Live DevTools repro: 10 rapid btn.click()s -> 1 message persists.
Single send still works after the burst resolves.
Send button briefly shows the disabled style during the in-flight RPC.

Notes

No new unit tests. The race is across event-loop ticks (synchronous closure body vs spawned future polling); reproducing it in cargo test would require a Dioxus runtime harness with a controllable executor. A unit test over Signal::set/Signal::peek would just retest the type's existing semantics.
Compatibility with PR #210: the own_message_ids insert + services::save_own_message_ids calls inside the spawned future run exactly once for the accepted send. No orphan SIDs from dropped clicks.
Empty-input guard is unchanged. The DevTools repro sets a non-empty value before each click, so the existing empty-guard wouldn't have caught this.
Enter-to-send: routed through the same props.on_send -> the gated handle_send, so it's covered by the same guard.

Out of scope

The sibling Button-component DOM-level disabled issue (re-introducing the HTML disabled attribute or otherwise hardening Button against synthetic .click() calls during the brief tick before re-render). The parent-level guard is the correct and sufficient fix for #183.

## Summary Rapid clicks on `.send-btn` (or repeated Enter) were spawning N concurrent `send_message` futures racing on the same client, with most resolutions silently dropped. The existing `sending.set(true)` ran *inside* the spawned future, so all clicks completed their synchronous closure body (clone + `spawn`) before any future was polled — N futures queued before the first one had a chance to flip `sending`. `Button`'s in-handler click-guard could not rescue this because the prop value the guard reads is only updated on the next render, which itself only happens after the parent flips `sending`. This PR adds a single synchronous re-entrancy guard at the top of `handle_send` and lifts `sending.set(true)` out of the spawned future so it runs synchronously, before `spawn`. The terminal `sending.set(false)` at the end of the future is unchanged and still covers both Ok and Err. ## Related Issue Closes https://forge.ourworld.tf/lhumina_code/hero_archipelagos/issues/183 ## Changes - **`archipelagos/messaging/src/archipelago.rs`**: in the `handle_send` closure - Added `if *sending.peek() { return; }` as the first statement (no subscription, synchronous read). - Moved `sending.set(true)` from inside the spawned future to *before* `spawn`, inside the `if let Route::Chat { sid, .. }` branch (so we don't latch `sending` when the route isn't a chat). - Existing `sending.set(false)` at the end of the future is unchanged. ``` archipelagos/messaging/src/archipelago.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) ``` ## Test Results - `cargo test -p hero_archipelagos_messaging --lib`: 5/5 passed (the PR #210 tests still green). - `cargo check -p hero_archipelagos_messaging`: clean. - `cargo check -p hero_archipelagos_messaging --target wasm32-unknown-unknown`: clean. - `cargo check --workspace --exclude hero_archipelagos_messaging`: clean (9 pre-existing `unused_mut` warnings in `server/src/main.rs` are unrelated). **Live verified in browser**: `make run` -> `http://localhost:8886` -> messaging DM. Pasted the issue's 10-click DevTools snippet, observed exactly 1 message bubble appear, F5, re-opened the chat, exactly 1 of 10 `rapid#N` messages persisted (vs 0 before the fix). Send button reflected the disabled state during the in-flight RPC. Subsequent single send worked normally. ## Test plan - [x] Build: native + wasm32 clean. - [x] PR #210 tests still pass. - [x] Live DevTools repro: 10 rapid `btn.click()`s -> 1 message persists. - [x] Single send still works after the burst resolves. - [x] Send button briefly shows the disabled style during the in-flight RPC. ## Notes - **No new unit tests.** The race is across event-loop ticks (synchronous closure body vs spawned future polling); reproducing it in `cargo test` would require a Dioxus runtime harness with a controllable executor. A unit test over `Signal::set`/`Signal::peek` would just retest the type's existing semantics. - **Compatibility with PR #210**: the `own_message_ids` insert + `services::save_own_message_ids` calls inside the spawned future run exactly once for the accepted send. No orphan SIDs from dropped clicks. - **Empty-input guard** is unchanged. The DevTools repro sets a non-empty value before each click, so the existing empty-guard wouldn't have caught this. - **Enter-to-send**: routed through the same `props.on_send` -> the gated `handle_send`, so it's covered by the same guard. ## Out of scope - The sibling Button-component DOM-level `disabled` issue (re-introducing the HTML `disabled` attribute or otherwise hardening Button against synthetic `.click()` calls during the brief tick before re-render). The parent-level guard is the correct and sufficient fix for #183.

rawdaGastan added 1 commit

2026-05-06 12:50:07 +00:00

fix(messaging): guard handle_send against re-entrancy to prevent dropped sends

Build and Test / build (pull_request) Successful in 7m10s

Details

0435c14681

Rapid clicks on the send button (or repeated Enter) used to spawn N
concurrent send_message futures racing on the same client, with most
resolutions silently dropped. The existing sending.set(true) ran inside
the spawned future, so all clicks completed their synchronous closure
body (clone + spawn) before any future was polled.

Fix: short-circuit synchronously at the top of handle_send via
*sending.peek(), and lift sending.set(true) out of the future so it
runs synchronously before spawn. The terminal sending.set(false) at
the end of the future is unchanged and still covers Ok and Err.

Verified live: 10 rapid btn.click()s now persist exactly 1 message.

#183