Make it so `HostController` manages both the module host (wasm
machinery) and the database (`RelationalDB` / `DatabaseInstanceContext`)
of spacetime databases deployed to a server.
The `DatabaseInstanceContextController` (DBIC) is removed in the
process.
This allows to make database accesses panic-safe, in that uncaught
panics will cause all resouces to be released and the database to be
restarted on subsequent access. This is a prerequisite for #985.
It also allows to move towards storage of the module binary directly in
the database / commitlog. This patch, however, makes some contortions in
order to **not** introduce a breaking change just yet.
* Create a lockfile when opening config files
In the past, we've had issues where multiple concurrent CLI processes
would race to read and write the CLI config file,
leading to data loss.
We considered using `flock`/`LockFileEx` and blocking until the file became available,
but unfortunately it's not possible to atomically create and lock a nonexistent file,
which we need to do in the case where the configuration doesn't yet exist.
Instead, we opt for a classic lockfile-based scheme:
Before opening a config file `foo.conf`, attempt to exclusively create `foo.lock`,
and panic if the exclusive creation fails.
Once it becomes clear that we will not write the config any more,
i.e. in `Config::drop`,
delete the lockfile, allowing another process to operate.
This means that attempting to run multiple concurrent Spacetime CLI processes
with the same config file is now a hard error.
* Fix CI failures
This commit fixes two CI failures:
- `spacetime start`, and a few other CLI subcommands, do not access their `Config` at all,
but the CLI constructs it unconditionally in `main`,
which made it an error to run any CLI command while `spacetime start` was running.
This is fixed by having subcommands which don't need a `Config`
drop it before doing anything.
- Contrary to my assumption,
the test configuration created by `Config::new_with_localhost` does get `drop`ped,
because the test harness `clone`s is and passes an owned version to the CLI.
This was causing it to attempt to delete the empty path, which failed.
This is fixed by having the home configuration be `Option`al,
and setting it to `None` in tests.
* Clap before config because they suppress destructors
Perform Clap argument parsing as the very first thing in a CLI process,
before locking the config,
because Clap calls `exit` directly on error rather than panicing
(presumably to have more control over error output),
which prevents destructors from running,
leaving stale lockfiles.
* Encapsulate lockfile logic in a type
Also deduplicate logic for finding config file paths.
* Define `create_parent_dir` helper with comments
* Replace `drop` calls with more explicit `Config::release_lock`.
This patch attempts to integrate the new commitlog with the minimum
changes.
Most of the diff comes from deletions of the legacy log and the need to
adjust tests due to the requirement for a tokio runtime when a durable
database is used in tests.
The "meat" of the patch are the `RelationalDB` constructors,
`RelationalDB::commit_tx`, and the replay logic in
`locking_tx_datastore`.
While `DataKey` is gone, there is still some redundant data being passed
around, which will be addressed in the follow-up patch.
This error turns out to have been a legitimate, albeit rare, race condition,
which could have (but apparently didn't) manifested in user code.
The cause, ultimately, was holding locks for too-short scopes
while initializing the connection.
It was possible for a new connection to receive and completely process
its `IdentityToken` message before calling `maybe_set_credentials`,
which would then clobber the received identity.
This is now fixed (hopefully) by holding the `CredentialStore` lock
for the duration of `BackgroundDbConnection::connect`.
Two other locks, on `ClientCache` and `ReducerCallbacks`,
are also newly held for the duration to avoid similar issues.
In addition, this commit adds a call to `disconnect` to the beginning of `connect`.
This will prevent pathological user code
which connects twice in a row without an intervening disconnect
from encountering bizarre errors
caused by the first connection's background workers staying active.
This commit also bumps the `RUST_LOG` env variable for the SDK test clients to `trace`,
and adds several trace-level logs to the SDK,
which were instrumental in pinning down the failure.
This does mean that log output of failed SDK tests will now be much noisier.
We've had continual issues with test isolation when developing breaking changes.
This commit doesn't fully address those, but is a step in the right direction:
the SDK tests now create a tempdir as their `STDB_PATH`,
rather than using `~/.spacetime`.
* Add `address: Address` to `ReducerContext`
Initial support for identifying connections via an `Address`,
passed as the caller address in `ReducerContext`.
Notable design choices:
- The same type `Address` is used for clients and dbs,
as opposed to having distinct `DbAddress` and `ClientAddress` types.
- For automatic reducers (init, scheduled, &c), the passed `Address`
is the database's address, not the address of the client which called `publish`.
- Clients may specify an `Address` as a query parameter
to the `subscribe` and `call` endpoints.
If they do not, an `Address` will be randomly generated for them.
Still to do:
- Send the client's `Address` alongside their `Identity` and token
upon WebSocket init in `IdentityToken`,
so the client can save its `Address` for re-connection.
- SDK support.
- C# module support.
- Documentation.
- Refuse new connections if an existing connection
with the same `(Identity, Address)` pair exists.
- Ensure we can store `Address` in tables,
so modules can track connected clients,
and ser/de `Address`, so those tables can be synced to clients.
* Use `None` in `ReducerContext` for HTTP calls without client address
* Fix typo in trait argument name
* `Address` representation amenable to SDK usage
Similar to `Identity`, make `Address` a product type with a double-underscored field name.
This will allow SDKs to distinguish `Address` from `Vec<u8>`,
but does necessitate some ugly `AddressForUrl` hackery
to get ser/de working correctly in different contexts.
This commit does not yet include SDK support for `Address`,
only the server-side machinery necessary for it.
* Add client_address parameter to /publish; pass to init and update reducers
Per discussion with Kim, it's useful for SpacetimeDB-cloud
for the client_address passed to the init reducer
to be that of the caller of /database/publish,
rather than the database's address.
This commit implements that behavior. Now:
- `init` and `update` take the client_address passed to publish, if any,
or `None` if no client_address was supplied.
- Scheduled reducers never receive a client_address.
- Reducers invoked by HTTP /call take the client_address passed to /call, if any,
or `None` if no client_address was supplied.
- Reducers invoked by WebSocket always receive the address of the WebSocket connection.
* `Address` support in Rust client SDK
* Add test that addresses are stable within a client process
* Run rustfmt against merge changes
* Not sure why my local `cargo fmt` didn't get this...
* Add caller_address argument to reducer arguments in Rust SDK
* rustfmt again...
* Add caller address to `EventJson` message
* Python codegen for client addresses
* C# module support for client addresses
* Fix scoping error
* Add `Address`-related stuff to C# codegen
- Emit `SpacetimeDB.Address` for address types
- Add `callerAddress` field to `ReducerEvent`
* TypeScript codegen changes
* Fix merge conflict with new test
* Run rustfmt
---------
Co-authored-by: Ingvar Stepanyan <me@rreverser.com>
* Client API changes to accomodate new cloud architecture
The exact trait interfaces for `client-api` is TBD
* Ensure we're not blocking when accessing the filesystem
* Derive Clone for SendGridController
* Add YOLO error variant to InsertDomainResult
* Rebase
* fixup! Rebase
* Fix SpacetimeType for Address
* Temporarily disable message / frame size limits for SDK WS
* Remove get_database_instance_state from API trait
It's an internal (worker db) thing, which does not need to be satisfied
by impls.
* Update indexes when updating a database
It turns out that the order of the index definitions in the proposed
schema may differ from those returned from the catalog, causing valid
(i.e. no-op) updates to be rejected. While at it, allow updating table
indexes so as long as the (column) schema remains unchanged.
* Update indexes when updating a database
It turns out that the order of the index definitions in the proposed
schema may differ from those returned from the catalog, causing valid
(i.e. no-op) updates to be rejected. While at it, allow updating table
indexes so as long as the (column) schema remains unchanged.
* Fix -S instead of -s for update-module smoke test
-s now means "server", -S "skip clippy", changed in:
a1e9984 (Multiple server configurations for CLI (#214), 2023-09-01)
* Invalidate schema cache when committing a tx
* Use long options in update-module.sh, fix unused warning
* Add test asserting schema_for_table reflects index updates
This switches tests from invoking a spacetimedb CLI found on PATH to always using up-to-date version by depending on the CLI crate as a library.
---------
Signed-off-by: Phoebe Goldman <phoebe@goldman-tribe.org>
Co-authored-by: Phoebe Goldman <phoebe@clockworklabs.io>
Noa