Unflushed commands before exclusive system when configured with *_ignore_deferred

[urben1680]

Bevy version

I saw this behavior at around 0.14 I think but only now got around making a minimal reproducible example on main, or at least a fairly recent commit behind main.

What you did

I ran this code:

use bevy::prelude::*;

#[derive(Resource)]
struct InsertTest;

fn insert_system(mut commands: Commands) {
    commands.insert_resource(InsertTest);
}
fn assert_system(world: &mut World) {
    assert!(world.contains_resource::<InsertTest>())
}

fn main() {
    App::new()
        .add_systems(
            Update, 
            (insert_system, assert_system).chain_ignore_deferred()
        )
        .update();
}

What went wrong

assert_system is an exclusive system, therefore it is expected that the command in insert_system is flushed before it runs. The assertion should not fail but it does.

Additional information

I did some extensive testing and noticed that this bug occurs no matter how I do this configuration as long I make use of the _ignore_deferred variants. The following table lists these configurations and gives each a number because that matters for another observation I elaborate further down.

inserting config by	asserting config by	config	test
system	system	after_ignore_deferred	❌ 1
system	system	before_ignore_deferred	❌ 2
system	system	chain_ignore_deferred	❌ 3
system	set	after_ignore_deferred	❌ 4
system	set	before_ignore_deferred	❌ 5
set	system	after_ignore_deferred	❌ 6
set	system	before_ignore_deferred	❌ 7
set	set	after_ignore_deferred	❌ 8
set	set	before_ignore_deferred	❌ 9
set	set	chain_ignore_deferred	❌ 10

For example the configuration...

(insert_system, assert_system).chain_ignore_deferred()

... is the third row in the table, where "inserting config by" with "system" means that the first element in the chain tuple is the system, and "asserting config by" with "system" means that the second element in the tuple is also the system itself.

"set" means if the argument is a set, so a configure_sets with this configuration...

(SetWithInsertSystem, SetWithAssertSystem).chain_ignore_deferred()

... is the last row.

chain cannot mix systems and sets, but before and after can mix, therefore these have more rows.

General observations regarding the two system:

• the tests do not fail if insert_system is exclusive
• the tests do not fail if insert_system is exclusive and assert_system is not exclusive

Third system affecting the outcome of the tests

Now I am adding a third, empty system and order them after the two above. I observed that this results in some test going green suddenly.

fn third() {}

insert-assert config by	third config by	config	✅ fixes the failing test of ...
each system	system	after	1 3 4 7 8 9 10
config tuple	system	before	1 3 4 7 8 9 10
config tuple	system	chain	4 7 8 9 10
each system	set	after	1 2 3 5 6 8 9 10
config tuple	set	before	1 3 4 7 8 9 10
set	system	after	1 2 3 4 5 7 8 9 10
set	system	before	1 2 3 5 6
set	set	after	1 2 3 5 6
set	set	before	1 2 3 5 6
set	set	chain	1 2 3 5 6

For example this is the first row with 3...

fn main() {
    App::new()
        .add_systems(
            Update, 
            (
                (insert_system, assert_system).chain_ignore_deferred(),
                third.after(insert_system).after(assert_system)
            )
        )
        .update();
}

... and this the second row with 3 ...

fn main() {
    App::new()
        .add_systems(
            Update, 
            (
                (insert_system, assert_system)
                    .chain_ignore_deferred().before(third),
                third
            )
        )
        .update();
}

... which both no longer fail.

General observations regarding the third system:

• it does not matter if third is exclusive or not
• If third is configured with _ignore_deferred variants, it does not affect the results of the first tests
• If third is put before the two, not after, it does not affect the result of the first tests

Test file

I generated the tests with this file which also tests many other combinations that do not fail. It might come useful for testing fixes for this issue. Sorry if it looks horribly, that is how I generate tests. 🫣 Also worth noting that I double-checked that like 5 times but mistakes may only come up with a sixth time. I am just comfortable enough to list the results.

[hukasu]

haven't looked at the source of bevy to know but my hypothesis is that taking &mut World is a sync point in and of itself

[alice-i-cecile]

Commands should always be flushed before and after an exclusive system. I think that assert should always be failing 🤔 Sys3's role here is really weird!

[hukasu]

are these the cases you have?

//! SILENCE

use bevy::prelude::*;

#[derive(Resource)]
struct InsertTest;

fn sys1(mut commands: Commands) {
    commands.insert_resource(InsertTest);
}
fn sys1_exclusive(world: &mut World) {
    world.insert_resource(InsertTest);
}

fn sys2(world: &mut World) {
    // this fails, the resource is already visible to this system
    assert!(!world.contains_resource::<InsertTest>());
}
fn sys2_non_exclusive(resource: Option<Res<InsertTest>>) {
    // this fails, the resource is already visible to this system
    assert!(resource.is_none());
}

fn sys3() {}

#[derive(SystemSet, Copy, Clone, Debug, Hash, PartialEq, Eq)]
struct MySet(u8);

fn base_app() -> App {
    let mut app = App::new();
    app.configure_sets(Update, (MySet(1), MySet(2)).chain());
    app
}

#[test]
#[should_panic]
fn three_systems() {
    base_app()
        .add_systems(
            Update,
            (sys1, sys2).chain_ignore_deferred().in_set(MySet(1)),
        )
        .add_systems(Update, sys3.in_set(MySet(2)))
        .update();
}

#[test]
#[should_panic]
fn three_systems_sys1_exclusive() {
    base_app()
        .add_systems(
            Update,
            (sys1_exclusive, sys2)
                .chain_ignore_deferred()
                .in_set(MySet(1)),
        )
        .add_systems(Update, sys3.in_set(MySet(2)))
        .update();
}

#[test]
#[should_panic]
fn three_systems_sys2_non_exclusive() {
    base_app()
        .add_systems(
            Update,
            (sys1, sys2_non_exclusive)
                .chain_ignore_deferred()
                .in_set(MySet(1)),
        )
        .add_systems(Update, sys3.in_set(MySet(2)))
        .update();
}

#[test]
#[should_panic]
fn three_systems_chained() {
    base_app()
        .add_systems(Update, (sys1, sys2, sys3).chain())
        .update();
}

#[test]
fn three_systems_chained_no_deferred() {
    base_app()
        .add_systems(Update, (sys1, sys2, sys3).chain_ignore_deferred())
        .update();
}

#[test]
fn two_systems() {
    base_app()
        .add_systems(Update, (sys1, sys2).chain_ignore_deferred())
        .update();
}

the ones with should_panic are the one that panic for me on 5e9da92

[hukasu]

again, as i and alice said, taking &mut World should be a sync point, so the failure here is actually the 2 cases that are not panicing

[urben1680]

Okay I spent some time doing extensive testing for the case of two systems and adding a third system. I updated the first post with all findings including a file that can be used to test fixes.
I deleted my previous comments with outdated assumptions. Note that I reversed the assertion, now panicking when the resource is missing if you want to update your comments.

[urben1680]

I skimmed through the bevy source to get an idea where the intended logic is put at.

Exclusive systems flush the world after every time they run in their implementation of System::run. This seems to be have added to support component hook lifecycles with #10756, not because it is a general behavior of exclusive systems.
EDIT: correction, World::flush does not apply the schedule's commands, only the global ones.

I looked at a field no_sync_edges that contains edges where sync points should be automatically inserted. (in contrast to actively adding it via ApplyDeferred I guess) That does not seem to care for exclusiveness of system nodes if you scroll through the file.

So I checked where System::is_exclusive matters.

Besides checking for conflicting access, it matters for when spawn_exclusive_system_task is run. But that only applies commands if the system is ApplyDeferred, which my assert_system is not. I also don't think that is where that behavior belongs as sync points should be independent of single/multi threading. Unless a fix is added to all executors.

Currently I run out of ideas where to look for this "commands are always applied before exclusive systems". I mean this issue shows that it is not really working.

I suspect my system third here is no hint where the bug is at but only adds it's own sync point. Though it is weird it is not added after assert_system in half the cases. Maybe because ExclusiveSystemFunction::has_deferred returns false so the scheduler does not think it matters where it adds the sync point.

[alice-i-cecile]

Currently I run out of ideas where to look for this "commands are always applied before exclusive systems". I mean this issue shows that it is not really working.

I don't think they currently are, but I do think they should be.

[urben1680]

I found another bug with the way automatic sync points are calculated in regard of *_ignore_deferred:

If there is a "pending" sync point at a node that itself has no deferred, even non-*_ignore_deferred configs will add no sync points:

#[derive(Resource, Default)]
struct TestRes;

App::new()
    .add_systems(
        Update,
        (
            (
                |mut commands: Commands| commands.init_resource::<TestRes>(),
                || () // has no deferred
            ).chain_ignore_deferred(), // last node still has no deferred
            |world: &mut World| assert!(world.contains_resource::<TestRes>()) // fails, does not need to be exclusive
        ).chain() // note that this is not `chain_ignore_deferred`!
    )
    .update();