Phoenix.Ecto.SQL.Sandbox plug not working reliably with Bandit

[michallepicki]

Hello!

When trying to switch from Cowboy to Bandit we found a possible incompatibility with plug Phoenix.Ecto.SQL.Sandbox. Our async API "integration" tests are working correctly with Cowboy, but fail in odd ways with Bandit. I made a repro repository using the Phoenix 1.8 rc installer from the phoenix master branch. Example failures on this repro look like:

$ mix test
Generated hello app
Running ExUnit with seed: 836115, max_cases: 32

  1) test create a few things, then list them all (HelloWeb.Integration.ABCDTest)
     test/hello_web/integration/abcd_test.exs:10
     match (=) failed
     The following variables were pinned:
       names = ["a", "b", "c", "d"]
     code:  assert ^names = Enum.map(response.body["data"], & &1["name"]) |> Enum.sort()
     left:  ^names
     right: ["a", "d", "f", "g"]
     stacktrace:
       test/hello_web/integration/abcd_test.exs:20: (test)

  2) test create a few things, then list them all (HelloWeb.Integration.EFGHTest)
     test/hello_web/integration/efgh_test.exs:10
     match (=) failed
     The following variables were pinned:
       names = ["e", "f", "g", "h"]
     code:  assert ^names = Enum.map(response.body["data"], & &1["name"]) |> Enum.sort()
     left:  ^names
     right: ["b", "c", "e", "h"]
     stacktrace:
       test/hello_web/integration/efgh_test.exs:20: (test)

Finished in 1.0 seconds (1.0s async, 0.00s sync)
2 tests, 2 failures

My suspicion is that since Bandit re-uses processes by default, when handling a request in a process that previously already handled a request for a different test, Phoenix's Ecto Sandbox plug will not switch from one sandbox to another. And setting http_1_options: [max_requests: 1], seemingly fixes the problem - but it feels wrong to do this change in tests because it's changing crucial config of our web server infrastructure which we do want to test as-is...

Maybe the plug should either switch from one sandbox to another when called? Or it should have a before_send that "disallows" the web handler process from using the sandbox it's allowing it to use?

cc @mtrudel

[josevalim]

I think http_1_options: [max_requests: 1] is the way to go, we should at least recommend that in the docs. Can you please:

1. Update the docs here to mention this option

2. Change Phoenix generators to include this option by default config/test.exs

WDYT?

[mtrudel]

I think what's happening here is exactly what you think: the fact that the HTTP handler process is reused means that when the Sandbox Plug decodes the metadata for the connection to use and tries to allow itself to access the given connection here, the sandbox returns {:already, :allowed} for the connection that was used on the previous request to this HTTP handler.

We ought to be able to resolve this by 'un-allowing' ourselves before trying to allow ourselves, in order to get the current handler process to stop using any connection it may already be using. This would work in both the case of the initial connection on a newly created checkout, as well as subsequent ones (though it would be a bit of extra work in the latter case). Unfortunately, I don't think that there's existing support for 'un-allowing' processes within Ecto.SQL (or DBConnection below it).

By my read, it looks like adding support for this to Ecto.SQL / DBConnection would be pretty straightforward, perhaps via a revoke_allow that would contain logic similar to checkin, but acting on :allowed processes instead of :owner processes (they manipulate the same checkouts table internally from what I can tell; I don't know the DBConnection codebase very well though).

Happy to take guidance on the sensibility of this from @josevalim or anyone else knowledgeable with DBConnection et al.

[josevalim]

Maybe we should approach this by having a "force" or "unallow_existing" option that removes all previous allowances whenever allowing a new request? This way we don't need any cleanup on the Bandit/Plug side.

[mtrudel]

Fix for this is a three parter:

• Add a new unallow_existing opt to db_connection ownership logic here
• Plumb through previously ignored options in Ecto.SQL.Adapters.Sandbox here
• Have Phoenix Ecto make use of the unallow_existing flag when allowing plugs to access a sandbox connection here

I've validated this set of PRs fixes the issue in the provided repro repo (thanks for that @michallepicki, it's like the single most valuable thing you can do in a bug report).

[josevalim]

Thank you, amazing work. I will do new db_connection, ecto, ecto_sql, and phoenix_ecto releases later today.

[mtarnovan]

Thank you all for the fast resolution! 🥇