Fix flaky subscription smoketests

[bfops]

Description of Changes

Fixes #1527. From there:

See e.g. https://github.com/clockworklabs/SpacetimeDB/actions/runs/9977698168/job/27575374257?pr=1479

I believe this is due to these smoketests using the CLI to subscribe to queries. They asynchronously read errors/exceptions out from the CLI output, but the CLI call is wrapped in a synchronous exception handler, so this handler may or may not be triggered properly depending on the timing. For example, in smoketests/tests/permissions.py:

        with self.assertRaises(Exception):
            self.subscribe("SELECT * FROM Secret", n=0)

Those blocks of code would create a subscription (in a background thread), but not necessarily wait for it to complete. I believe there was a small race condition where we could exit the assertRaises block before getting the exception (see the logs in the linked ticket).

I have updated these calls to wait for the subscription to finish, which re-raises any exceptions that occur in the background thread. Note this is due to subscribe using a custom thread class - this re-raising does not happen by default in Python.

API and ABI breaking changes

No

Expected complexity level and risk

1

Testing

The CI currently passes, so it's not obviously-broken.

I was not able to reproduce the failure locally (at least, for the amount of time I was willing to wait). So, my only testing is that the tests still pass. It's probably not more flaky.

If anybody else wants to try, this was the loop I used:

true;
while [[ $? == 0 ]]; do
  python3 -m smoketests;
done;
discord-ping 'test failed';

[bfops]

adding this () causes us to wait for the subscription results (which, in this case, will throw an exception).

This is the same for the lines below.

[joshua-spacetime]

I did not actually address the race condition in the subscribe function.

@bfops what's the race condition and why does this change make it so that we're no longer relying on it?

[Centril]

Woops, makes sense 👍

[bfops]

I did not actually address the race condition in the subscribe function.

@bfops what's the race condition and why does this change make it so that we're no longer relying on it?

I updated the PR description with the details from the issue as well. Does the current explanation make sense?

tl;dr we were creating a subscription but not waiting for it to complete within the assertRaises block.

Dismissing because Josh has outstanding questions and I don't want to accidentally merge without addressing those

[bfops]

This was the source of the race condition - if the CLI process failed quickly enough, we would raise this exception synchronously in the call to subscribe. But if the process took a bit longer to fail, then the block below would be the one to raise the exception, which would only be raised to the caller when the caller actually joined the background thread and waited for the subscription to complete.

By removing this block, we now only get exceptions when we wait for the subscription result. We will never get the exception synchronously. This eliminates the race condition - we must always wait for the subscription to complete in order to see the exception.

I have updated the tests accordingly.

[joshua-spacetime]

Excellent!

[joshua-spacetime]

Excellent!