Skip to content

Fix panic-in-drop in future tests; harden against race conditions - #1617

Merged
Bromeon merged 3 commits into
masterfrom
bugfix/signal-future-drop-freed-object
May 23, 2026
Merged

Bromeon merged 3 commits into
masterfrom
bugfix/signal-future-drop-freed-object

Conversation

@Bromeon

@Bromeon Bromeon commented May 23, 2026 •

Copy link
Copy Markdown
Member

In CI workflow runs A, B and C, job godot-itest (linux-features-experimental) failed with a process abort (core dump). The Godot process printed:

thread caused non-unwinding panic. aborting.

...but without any panic payload. It turned out to be a latent race condition in godot-rust; I could reproduce it on Godot version fa6cad230. Running tests filtered by [async,signal_] on two cores (taskset -c 0,1) let the abort appear in ~1/76 runs.

When a SignalFuture is dropped, FallibleSignalFuture::drop disconnects its ONE_SHOT resolver via Signal::object(). If the signal's object is freed concurrently on another thread, that access panics -- and a panic inside Drop becomes a non-unwinding abort.

This was only reachable through unsafe, since Gd is !Send and must be bypassed to free it on another thread. In our case it's the ThreadCrosser.


This PR applies multiple fixes and hardenings:

  1. Skip cleanup when nothing needs disconnecting. drop only touches the object while the future is still Pending; once the signal fired, the ONE_SHOT connection is already gone. This eliminates the reproduced abort.
  2. Fix Signal::object() not properly catching dead objects. We now use is_instance_valid() on the object returned by Godot's own Signal::get_object API and return None for dead objects, instead of panicking (which could happen in Drop).
  3. Contain remaining panics in Drop. For the rare Pending-and-freed-concurrently TOCTOU race, cleanup is wrapped in handle_panic. Instead of aborting, we skip the disconnect() and leave the connection registered. Godot will disconnect all object's signal connections when the object is destroyed, releasing the reference it holds to our resolver Callable.

`FallibleSignalFuture::drop` unconditionally accessed the signal's object to
disconnect the resolver callable. When the signal had already fired (the
common case under `ONE_SHOT`), the connection was already gone, so the access
was unnecessary -- and unsound when the object had been freed concurrently
(e.g. a signal emitted from another thread that then drops the last reference).
Touching a freed object inside `Drop` panics in `maybe_inc_ref` and escalates
to a fatal non-unwinding abort.

Now the drop only attempts disconnection while the future is still `Pending`;
once `Ready`/`Dead`, it skips the object access entirely.

Additionally, `Signal::object()` now honors its documented contract of
returning `None` for dead objects instead of panicking, by validating instance
liveness before incrementing the refcount.
@Bromeon Bromeon added bug c: async Everything related to async tasks/signals labels May 23, 2026
Bromeon added 2 commits May 23, 2026 20:16
The test deliberately drops the only reference to the signal's object on the
worker thread, so the object is freed while the SignalFuture is still being
dropped on the main thread. This is the exact scenario that previously aborted
the process. Add a comment so the test is not later "fixed" by keeping the
object alive, which would silently remove the regression coverage.
When the future is dropped before the signal fired, the still-live ONE_SHOT
connection must be disconnected, which requires accessing the signal's object.
That object can be freed concurrently -- reachable only when a `Gd` is moved
across threads via the `unsafe` cross-thread contract (`Gd` is `!Send`) and
freed there. The resulting access panics, and a panic escaping `Drop` aborts
the process.

Wrap the cleanup in `handle_panic` so such a race is contained: the stale
connection is leaked (Godot removes it when the object dies) instead of
aborting. The common case (object alive) disconnects normally. Update the
`SignalFuture`/`FallibleSignalFuture` docs accordingly.
@Bromeon
Bromeon force-pushed the bugfix/signal-future-drop-freed-object branch from f4544c5 to f351fb2 Compare May 23, 2026 18:17
@GodotRust

Copy link
Copy Markdown

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-1617

@Bromeon
Bromeon added this pull request to the merge queue May 23, 2026
Merged via the queue into master with commit f18e826 May 23, 2026
23 checks passed
@Bromeon
Bromeon deleted the bugfix/signal-future-drop-freed-object branch May 23, 2026 19:46
TW0hank0 added a commit to TW0hank0/positive_mahjong that referenced this pull request Aug 19, 2026
Update patch dependencies

This MR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [console_error_panic_hook](https://github.com/rustwasm/console_error_panic_hook) | workspace.dependencies | patch | `=0.1.5` β†’ `=0.1.7` |
| [godot](https://godot-rust.github.io) ([source](https://github.com/godot-rust/gdext)) | workspace.dependencies | patch | `=0.5.3` β†’ `=0.5.5` |
| [serde](https://serde.rs) ([source](https://github.com/serde-rs/serde)) | workspace.dependencies | patch | `=1.0.228` β†’ `=1.0.229` |
| [serde_json](https://github.com/serde-rs/json) | workspace.dependencies | patch | `=1.0.149` β†’ `=1.0.151` |

---

### Release Notes

<details>
<summary>rustwasm/console_error_panic_hook (console_error_panic_hook)</summary>

### [`v0.1.6`](rustwasm/console_error_panic_hook@0.1.5...0.1.6)

[Compare Source](rustwasm/console_error_panic_hook@0.1.5...0.1.6)

</details>

<details>
<summary>godot-rust/gdext (godot)</summary>

### [`v0.5.5`](https://github.com/godot-rust/gdext/blob/HEAD/Changelog.md#v055)

[Compare Source](godot-rust/gdext@v0.5.4...v0.5.5)

*9 August 2026*

##### 🌻 Features

- `EngineBitfield`: add `with()` and `without()` methods ([#&#8203;1627](godot-rust/gdext#1627))
- `#[func(virtual)]` now supports `async fn` for GDScript coroutines ([#&#8203;1645](godot-rust/gdext#1645))
- Add `PhantomData<T>` support for `GodotConvert` derive macro ([#&#8203;1619](godot-rust/gdext#1619))

##### πŸ“ˆ Performance

- Singletons are now cached ([#&#8203;1636](godot-rust/gdext#1636))
- Cache user singleton pointers ([#&#8203;1644](godot-rust/gdext#1644))
- Cheaper refcount operations ([#&#8203;1669](godot-rust/gdext#1669))

##### 🧹 Quality of life

- Always use godot\_error in panic hook ([#&#8203;1647](godot-rust/gdext#1647))
- Rename `cast_int/cast_float` to `to_vector*/to_rect*` ([#&#8203;1661](godot-rust/gdext#1661))
- Validate class registrations against `ClassDB` ([#&#8203;1664](godot-rust/gdext#1664))

##### πŸ› οΈ Bugfixes

- Fix `#[var(pub)]` panic during `OnEditor<Gd<T>>` reload ([#&#8203;1639](godot-rust/gdext#1639))
- Fix async `SignalFuture` panic when object is freed before exit flag ([#&#8203;1648](godot-rust/gdext#1648))
- Fix `api-custom` build when prebuilt lags detected Godot version ([#&#8203;1649](godot-rust/gdext#1649))
- Harden various parts of codebase ([#&#8203;1660](godot-rust/gdext#1660))
- Fix `GFile` `BufRead` past EOF ([#&#8203;1670](godot-rust/gdext#1670))

##### πŸ—οΈ Maintenance

- Replace `godot-cell` borrow-state proptests with exhaustive model check ([#&#8203;1650](godot-rust/gdext#1650))
- Make accidental access of `#[doc(hidden)]` symbols more explicit ([#&#8203;1651](godot-rust/gdext#1651))
- Follow-up to rustc / Godot changes ([#&#8203;1659](godot-rust/gdext#1659))
- Name virtual-callback layers after user-facing `I*` virtuals ([#&#8203;1662](godot-rust/gdext#1662))
- Move ref-counting operations from `DynMemory` to `RawGd` ([#&#8203;1667](godot-rust/gdext#1667))

### [`v0.5.4`](https://github.com/godot-rust/gdext/blob/HEAD/Changelog.md#v054)

[Compare Source](godot-rust/gdext@v0.5.3...v0.5.4)

*23 June 2026*

##### 🌻 Features

- Godot 4.7: add `api-4-7` level ([#&#8203;1641](godot-rust/gdext#1641))
- Type-Safe API for User-defined RPCs ([#&#8203;1535](godot-rust/gdext#1535), [#&#8203;1634](godot-rust/gdext#1634))
- String conversions: ` to_gstring()`, `to_string_name()` etc ([#&#8203;1623](godot-rust/gdext#1623))
- Thread-safe FFI scaffolding + cross-thread printing ([#&#8203;1632](godot-rust/gdext#1632))
- `Gd` introspection APIs: `dynamic_class`, `is_dynamic_class[_of]`, `is_ref_counted` ([#&#8203;1631](godot-rust/gdext#1631))
- Support async versions of `tools::load()` and `tools::try_load()` ([#&#8203;1601](godot-rust/gdext#1601))
- Support `@export_node_path` attribute ([#&#8203;1620](godot-rust/gdext#1620))

##### 🧹 Quality of life

- Slightly more flexible `GodotStringExt` through unsized impls ([#&#8203;1629](godot-rust/gdext#1629))
- Suppress Godot errors in fallible functions ([#&#8203;1637](godot-rust/gdext#1637))

##### πŸ› οΈ Bugfixes

- Fix Linux hot-reload panic when TLS destructors fire before init ([#&#8203;1611](godot-rust/gdext#1611))
- Fix panic-in-drop in future tests; harden against race conditions ([#&#8203;1617](godot-rust/gdext#1617))
- Auto-remove custom callables also when connected in untyped APIs ([#&#8203;1612](godot-rust/gdext#1612))
- Fix async panics at shutdown ([#&#8203;1625](godot-rust/gdext#1625))
- Fix `Object` returns with dynamic type `RefCounted` ([#&#8203;1628](godot-rust/gdext#1628))
- Fix memory leak for `connect_self()` on `RefCounted` ([#&#8203;1635](godot-rust/gdext#1635))
- Fix dangling singleton pointer after deinit ([#&#8203;1638](godot-rust/gdext#1638))
- Fix RPC borrow panic for `call_local` ([#&#8203;1643](godot-rust/gdext#1643))

##### πŸ—οΈ Maintenance

- Add `once` flag to `defer_startup_warn!` and `defer_startup_error!` ([#&#8203;1613](godot-rust/gdext#1613))
- CI: unzip occasionally fails; retry ([#&#8203;1616](godot-rust/gdext#1616))
- Rename self-registration parts: `plugin` -> `shard` ([#&#8203;1618](godot-rust/gdext#1618))
- Replace internal `PassiveGd` with safer `BorrowedGd` ([#&#8203;1633](godot-rust/gdext#1633))

##### πŸ“š Documentation

- Document `register-docs` security; warn when used in export builds ([#&#8203;1615](godot-rust/gdext#1615))
- Document `#[class(tool)]` + `ready()` behavior across editor reload ([#&#8203;1614](godot-rust/gdext#1614))
- Fix borrow panic in `godot::task::spawn` doc example ([#&#8203;1622](godot-rust/gdext#1622))

</details>

<details>
<summary>serde-rs/serde (serde)</summary>

### [`v1.0.229`](https://github.com/serde-rs/serde/releases/tag/v1.0.229)

[Compare Source](serde-rs/serde@v1.0.228...v1.0.229)

- Update to syn 3

</details>

<details>
<summary>serde-rs/json (serde_json)</summary>

### [`v1.0.151`](https://github.com/serde-rs/json/releases/tag/v1.0.151)

[Compare Source](serde-rs/json@v1.0.150...v1.0.151)

- Add RawValue::from\_string\_unchecked ([#&#8203;1331](serde-rs/json#1331), thanks [@&#8203;WonderLawrence](https://github.com/WonderLawrence))

### [`v1.0.150`](https://github.com/serde-rs/json/releases/tag/v1.0.150)

[Compare Source](serde-rs/json@v1.0.149...v1.0.150)

- Reject non-string enum object keys ([#&#8203;1324](serde-rs/json#1324), thanks [@&#8203;puneetdixit200](https://github.com/puneetdixit200))

</details>

---

### Configuration

πŸ“… **Schedule**: (in timezone Asia/Taipei)

- Branch creation
  - At any time (no schedule defined)
- Automerge
  - At any time (no schedule defined)

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

β™» **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

πŸ‘» **Immortal**: This MR will be recreated if closed unmerged. Get [config help](https://github.com/renovatebot/renovate/discussions) if that's undesired.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Mend Renovate CLI](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0NC4zMy4yIiwidXBkYXRlZEluVmVyIjoiNDQuMzMuMiIsInRhcmdldEJyYW5jaCI6Im1hc3RlciIsImxhYmVscyI6W119-->

See merge request TW0hank0/positive_mahjong!142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug c: async Everything related to async tasks/signals

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants