Implement typed publish-only database environments - #5887
Conversation
| if (tag == 0) { // Some, matching the canonical BSATN option type. | ||
| return SpacetimeDB::bsatn::deserialize<T>(*this); | ||
| } else if (tag == 1) { // None. | ||
| return std::nullopt; |
There was a problem hiding this comment.
Looks like the tags are being reordered here, was this a bug with the old code?
There was a problem hiding this comment.
Yes. The old C++ reader had these reversed: canonical BSATN uses tag 0 for Some and tag 1 for None. This fixes the reader without changing the wire format. The regression covers missing, present-empty, and embedded-NUL values, and checks that the following field is still decoded correctly.
cloutiertyler
changed the title
| #[serde_as] | ||
| #[derive(Clone, Serialize, Deserialize)] | ||
| #[serde(deny_unknown_fields)] | ||
| pub struct PublishRequest { |
There was a problem hiding this comment.
Is this an API breaking change? Should we consider just using HTTP headers instead of putting these in a map in the body?
There was a problem hiding this comment.
Old clients remain supported by the new server: the JSON envelope is selected by application/vnd.spacetimedb.publish+json, while raw module bodies still work and supply an empty env map. There is a compatibility gap in the other direction: this CLI currently sends the envelope even for modules without env declarations. I'll retain raw-body publishing for those modules so they can still target older servers.
I would keep env values in the body. We allow up to 256 values of 8 KiB each, which is too large for typical HTTP header limits, and values can contain characters unsuitable for headers. A separate body format also keeps module bytes and the complete env map in one publish request.
There was a problem hiding this comment.
Alright, I suppose. It just makes publishing more complicated for people who are currently publishing via HTTP directly (imagine publishing from a module in a procedure for example). We could also decrease the maximum allowed env value size, or otherwise decrease the number of env variables, or maybe just set a total size limit.
There was a problem hiding this comment.
Agreed. A lower aggregate limit on the encoded environment would make headers feasible; the current limits are a design choice, not a reason headers are impossible. The body format adds base64/JSON work for direct HTTP publishers, including procedures, and I should have weighed that more explicitly. Raw-body publishing still works for requests without ENV values. I'll document the direct HTTP example and the header alternative, including the aggregate limit and encoding it would require, rather than treating the current envelope as inevitable.
There was a problem hiding this comment.
Where will you document it?
There was a problem hiding this comment.
In the existing HTTP database API reference, under a new "Publishing with environment values" subsection covering both POST and PUT. I've written the content type, JSON shape, complete-replacement rules, compatibility behavior, and a complete Python example. The ENV guide links to it. I also added the HTTP-header alternative and its encoding/aggregate-size tradeoffs to proposal #3942.
There was a problem hiding this comment.
Why a Python example?
There was a problem hiding this comment.
I chose Python because its standard library made the Base64 and JSON encoding easy to show in one script; it was an arbitrary example choice. I replaced it with a curl/jq example in the HTTP API reference and kept the exact wire format explicit for procedures and other HTTP clients. The example preserves quotes, newlines, and Unicode in values.
| // TODO: Review log level after user SQL errors can be distinguished from internal database failures. | ||
| log::warn!("{e}"); | ||
| // Parser diagnostics can quote values. Return them only to the caller. | ||
| log::warn!("SQL request rejected"); |
There was a problem hiding this comment.
Note for other reviewers: are we cool with just not logging this?
There was a problem hiding this comment.
The intent is to keep submitted SQL and parser diagnostics out of shared logs because either can contain secret values. We still log the SQL byte count and a generic rejection warning, and return the detailed error to the caller. This does reduce diagnostic detail; a structured error category would let us recover some of it without recording query text or values.
There was a problem hiding this comment.
I don't think the log level should be warn here. I think it should be debug.
There was a problem hiding this comment.
Changed the rejection message to debug. It still omits SQL text and parser diagnostics; the caller receives the detailed error.
There was a problem hiding this comment.
Note for @coolreader18. The changes in this file seem a little complex to my eyes. I'm wondering if there's a better way to manage this.
There was a problem hiding this comment.
There are a few separable changes here: passing the complete environment through publication, loading initial values only for a new database, and preserving the running host when publication fails. The cleanup additions came from actual rejected-publication and reset tests: an error could leave the host registry empty, and an unused candidate could wait on a scheduler that was never started. I agree the control flow could be clearer. A small candidate-cleanup helper would reduce duplication while retaining those guarantees.
There was a problem hiding this comment.
My understanding is that the changes in this file make it so that a view that panics returns an error, rather than an empty view.
I'm not sure what the intended behavior @joshua-spacetime had in mind. I could see it going either way.
There was a problem hiding this comment.
Yes. This changes ordinary SQL and subscription materialization to return an error when a view fails, instead of continuing with its backing table. That is a broader behavior change than environment support. I have not reproduced it on unmodified master or established that an empty result violates the intended contract, so I would separate this change and confirm the intended behavior with Joshua.
| UpdateDatabaseResult::ErrorExecutingMigration(anyhow::anyhow!(msg)) | ||
| } else { | ||
| let tx_offset = succeed(self.info.clone(), out.execution_budget_used, out.total_duration, tx); | ||
| effects.committed(tx_offset, durable_offset) |
There was a problem hiding this comment.
This is also related to that trap bug.
There was a problem hiding this comment.
This part is separate from the empty-view/error change. A publish can change ENV and require client disconnection at the same time. The old branch skipped refreshing views in that case, including materializations created by ordinary SQL with no live subscriber to disconnect. This preserves client disconnection while refreshing surviving views against the newly published environment in the same transaction. I would keep that ENV consistency fix separate from the broader trap behavior changes.
| metrics_registry.register(Box::new(&*DATA_SIZE_METRICS)).unwrap(); | ||
|
|
||
| Ok(Arc::new(Self { | ||
| Ok(Arc::new_cyclic(|weak_self| Self { |
There was a problem hiding this comment.
Why is this now required?
There was a problem hiding this comment.
The new spawned publish/reset/delete and leader-start tasks need an owned Arc<StandaloneEnv>, but their trait methods receive only &self. The stored Weak<Self> lets them acquire that ownership without a strong reference cycle. The purpose is to keep the publication lock held until accepted work actually finishes, even if the HTTP waiter is cancelled. Arc::new_cyclic is a consequence of this task-ownership design, not a requirement of environment storage itself.
| let database_identity = database.database_identity; | ||
|
|
||
| let leader = self.leader(database_id).await?; | ||
| let leader = self.leader_under_publication(database_id).await?; |
There was a problem hiding this comment.
What is leader under publication?
There was a problem hiding this comment.
It means 'look up or start the leader while the caller already holds the publication lock.' Normal leader() acquires a read lock; publish/reset already holds the write lock, so calling it there would deadlock. This helper performs the same lookup/start without acquiring the lock again. The name is unclear; I'll rename it to state that the publication lock is already held.
| Ok(()) | ||
| } | ||
|
|
||
| async fn schedule_replicas(&self, database_id: u64, num_replicas: u8) -> Result<(), anyhow::Error> { |
There was a problem hiding this comment.
Why are we getting rid of schedule_replicas?
There was a problem hiding this comment.
The old creation path inserted the database first, then created the replica records in schedule_replicas. The new path persists the database, initial environment, and leader replica together before launching the host, so recovery cannot see a database without its bootstrap input or leader record. Standalone still uses one replica. The launch step remains in on_insert_replica; only the separate record-creation step is replaced.
There was a problem hiding this comment.
There's a lot of code motion in this file and just by vibes it doesn't all feel necessary.
There was a problem hiding this comment.
The main move separates the trait methods from operation bodies so a spawned task owns the publication lock through completion after caller cancellation. That ownership matters for reset versus concurrent startup. It does not justify unrelated reshuffling. I'll keep the cancellation requirement explicit and trim the diff to the smallest extraction needed for it.
| API_KEY='development-only-key' spacetime publish | ||
| ``` | ||
|
|
||
| For real credentials, supply the value through the publishing process's environment or an appropriately ignored local configuration file. Keep secrets out of checked-in configuration and module source. |
There was a problem hiding this comment.
Maybe we should suggest which of the various configuration files should contain secrets? I think basically any *.local.* file is not meant to be checked in, right?
There was a problem hiding this comment.
Yes. I'll explicitly recommend spacetime.local.json or spacetime.{environment}.local.json for local secrets and show the corresponding .gitignore entries. The .local convention indicates personal configuration, but the documentation should still tell users to ensure those files are ignored. Checked-in spacetime.json and spacetime.{environment}.json should contain only non-secret defaults.
There was a problem hiding this comment.
Added the specific recommendation to keep secrets in spacetime.local.json or spacetime.{environment}.local.json, together with the .gitignore patterns. Checked-in configuration is documented as containing non-secret defaults.
| </TabItem> | ||
| </Tabs> | ||
|
|
||
| The string-key getter checks the same declaration and permissions as named accessors. Reading an undeclared key fails; it does not return an absent value. Named optional accessors return `None`, `undefined`, `null`, or `std::nullopt`, depending on the language. The TypeScript string-key getter uses `null` for absence. |
There was a problem hiding this comment.
I feel like it would be better to have some minor duplication and put these comments inside each specific language.
For example:
- C#:
The string-key getter checks the same declaration and permissions as named accessors. Reading an undeclared key fails; it does not return an absent value. Named optional accessors return `null`.
Key names are exact and case-sensitive. The getter name `get`, or `Get` in C#, is reserved: a key with that name remains accessible through the string-key getter. A module with no environment declarations accepts no keys.
Reads inside a transaction use that transaction's snapshot. In a procedure outside a transaction, each read uses a separate snapshot. To read several keys consistently, group the reads in `WithTx`.
- TypeScript:
The string-key getter checks the same declaration and permissions as named accessors. Reading an undeclared key fails; it does not return an absent value. Named optional accessors return `null`.
Key names are exact and case-sensitive.
Reads inside a transaction use that transaction's snapshot. In a procedure outside a transaction, each read uses a separate snapshot. To read several keys consistently, group the reads in `withTx`.
There was a problem hiding this comment.
Moved the accessor, reserved-name, and transaction-snapshot explanations into each language tab. The TypeScript tab explicitly distinguishes undefined from the optional named accessor and null from the string-key getter; the other tabs now use their own return types and transaction method names.
|
|
||
| The configuration files `spacetime.json`, `spacetime.local.json`, `spacetime.{environment}.json`, and `spacetime.{environment}.local.json` apply in increasing precedence, where _environment_ is the environment selected with `--env`. Their `env` maps merge by key, as do maps inherited by child database targets. A higher-precedence value replaces that key while preserving unrelated keys. An empty map does not erase inherited keys. | ||
|
|
||
| JSON strings pass through unchanged. Booleans and numbers are converted to strings, so `false` supplies `"false"`; declarations still validate strings. Use JSON strings when exact numeric spelling matters. Arrays, objects, and `null` are rejected, as are JSON keys the module has not declared. An invalid effective value rejects the publish rather than falling back to a lower-precedence value. |
There was a problem hiding this comment.
Since null is rejected, maybe we can be extra explicit and explain to omit the value in the JSON file instead of using a null value for optional declarations. It's clear as it is, but maybe it doesn't hurt to add it.
There was a problem hiding this comment.
Added an explicit instruction to omit an optional key from the JSON object rather than assign null. The text also points out that any inherited configuration or shell value must be removed to make the effective value absent.
|
|
||
| ## Access and limits | ||
|
|
||
| Reducers, procedures, views, and HTTP handlers entered by the host in the root module can read its declared environment. Host-dispatched submodule entry points cannot read it, and submodules cannot declare a nonempty environment. Ordinary helper calls retain their calling entry point's access, including helpers defined in libraries or submodules. Root code can also pass a value to a helper explicitly. |
There was a problem hiding this comment.
We're clarifying that submodules can't access the root's env variables. But can submodules have their own secrets? I don't think they can, but if they can:
- how do you set them up?
- can the root module access them?
If they can't, maybe we should mention it here and we should definitely update the submodule documentation because I remember that in there we wrote something like "submodules are the same as any regular module and can be published independently, the only difference is that they can't have Lifecycle Reducers" (and now we should add env variables too).
There was a problem hiding this comment.
Submodules cannot declare a nonempty environment, and there is no separate submodule environment map to configure. A module with ENV declarations can be published independently as a root module, but cannot be included as a submodule with those declarations. I clarified this in both the ENV guide and the submodule documentation, including passing configuration explicitly from root code to helpers.
|
|
||
| Update that table through reducers that explicitly authorize the caller. Keeping a table private controls direct client reads; it does not authorize calls to a reducer that modifies or returns its contents. Apply the same care to views, procedure results, and logs. Private tables follow the database's normal private-table permissions, including administrative reads. | ||
|
|
||
| Unlike environment variables, these values follow the table's ordinary update and migration behavior. They do not receive environment schema validation or complete replacement on every publish. |
There was a problem hiding this comment.
Private tables are much more convenient (at least in my opinion). Should we add a warning box highlighting the potential risk of using a private table just for convenience or lazyness when the right tool is indeed an environment variable instead?
There was a problem hiding this comment.
Nah, private tables are fine to use. Env variables are just meant as a convenience. It's kind of a problem if they're not as convenient as private tables!
Although I think you may be discounting the fact that they're guaranteed to be defined in init, etc.
There was a problem hiding this comment.
Kept private tables as a supported option without adding a warning box. The guide now calls out the additional ENV guarantee: required values are validated and available before init or migration runs.
lisandroct
left a comment
lisandroct
left a comment
There was a problem hiding this comment.
Extremely minor change to handle some collisions with other methods in C# (besides the one that are already being handled).
| Assert.True(emitted.Success, string.Join("\n", emitted.Diagnostics)); | ||
| var assembly = Assembly.Load(stream.ToArray()); | ||
| assembly.GetType("Usage")!.GetMethod("Check")!.Invoke(null, null); | ||
| Assert.Null(assembly.GetType("SpacetimeDB.ModuleEnvironment")!.GetProperty("Get")); |
There was a problem hiding this comment.
Probably not necessary, but if an environment variable is called Finalize, GetHashCode, GetType, MemberwiseClone or ToString (which are all valid names), they would collide with methods in the Object type in C#. So we could handle and skip them like we skip "Get" here.
There was a problem hiding this comment.
Added a compilation regression for all eight reserved accessor names, including Finalize, GetType, and MemberwiseClone. It verifies that their declarations and string-key access remain available while conflicting named properties are omitted. The focused Codegen suite passes all 15 cases.
| ); | ||
| // Preserve the checked generic method, including a key literally | ||
| // named Get. Keywords are escaped without renaming stored keys. | ||
| if (name is "Get" or "ModuleEnvironment" or "Equals" or "GetHashCode" or "ToString") |
There was a problem hiding this comment.
Ahhh, you're handling some of the ones I mentioned in the test above. Since we're already handling these, we should also add Finalize, GetType and MemberwiseClone.
There was a problem hiding this comment.
Added Finalize, GetType, and MemberwiseClone to the reserved-name check. The new regression failed on exactly those three names before the fix and passes afterward.
2 participants
Description of Changes
Implements typed, publish-only environment variables: declared schemas, typed context accessors, CLI configuration resolution, and atomic replacement with the module on publish. Values are private and read-only to module code. Includes an ENV usage guide. Companion: SpacetimeDBPrivate#3940.
API and ABI breaking changes
Adds environment declarations to V10 metadata and new host imports. Modules using these additions require an updated host. No V11 ABI.
Rollback safety impact
n/a (no prerequisite PRs).
Hosts without ENV support cannot load modules using the new metadata. Rolling back those databases requires a migration.
Expected complexity level and risk
4/5. Changes span publication, durable storage, transaction consistency, secret access controls, and all four module libraries.
Testing
CLI smoke tests, schema validation, typed accessors, atomic publish/rollback, view refresh, submodule access restrictions, and restart/recovery tests.