Skip to content

Implement typed publish-only database environments - #5887

Open
cloutiertyler wants to merge 20 commits into
masterfrom
tyler/environment-variables
Open

Implement typed publish-only database environments#5887
cloutiertyler wants to merge 20 commits into
masterfrom
tyler/environment-variables

Conversation

@cloutiertyler

@cloutiertyler cloutiertyler commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

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.

if (tag == 0) { // Some, matching the canonical BSATN option type.
return SpacetimeDB::bsatn::deserialize<T>(*this);
} else if (tag == 1) { // None.
return std::nullopt;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the tags are being reordered here, was this a bug with the old code?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/bindings-csharp/Runtime/Internal/FFI.cs
@cloutiertyler cloutiertyler changed the title Add database environment storage, SQL, and module bindings Implement typed publish-only database environments Sep 8, 2026
Comment thread crates/bindings-sys/src/lib.rs
Comment thread crates/bindings/src/lib.rs
Comment thread crates/bindings-cpp/src/abi/wasi_shims.cpp Outdated
Comment thread crates/bindings-cpp/src/abi/wasi_shims.cpp Outdated
Comment thread crates/bindings-csharp/Runtime/build/SpacetimeDB.Runtime.targets Outdated
Comment thread crates/bindings-macro/src/environment.rs Outdated
Comment thread crates/bindings-typescript/src/server/environment.ts Outdated
Comment thread crates/cli/src/spacetime_config/environment.rs
Comment thread crates/cli/src/subcommands/env.rs
Comment thread crates/cli/src/subcommands/publish.rs Outdated
Comment thread crates/cli/src/subcommands/publish/environment.rs Outdated
Comment thread crates/cli/src/subcommands/publish/environment/tests.rs Outdated
#[serde_as]
#[derive(Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct PublishRequest {

@cloutiertyler cloutiertyler Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an API breaking change? Should we consider just using HTTP headers instead of putting these in a map in the body?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where will you document it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a Python example?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/client-api/src/lib.rs Outdated
// 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");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for other reviewers: are we cool with just not logging this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the log level should be warn here. I think it should be debug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the rejection message to debug. It still omits SQL text and parser diagnostics; the caller receives the detailed error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread crates/core/src/host/v8/syscall/common.rs
Comment thread crates/core/src/host/v8/syscall/mod.rs Outdated
Comment thread crates/core/src/host/wasm_common.rs Outdated
Comment thread crates/core/src/host/wasm_common/module_host_actor.rs
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)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also related to that trap bug.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread docs/docs/00200-core-concepts/00100-databases/00700-environment-variables.md Outdated
Comment thread modules/module-test-ts/src/index.ts Outdated
Comment thread modules/module-test-ts/src/lib_submodule.ts
Comment thread crates/bindings-cpp/src/abi/wasi_shims.cpp Outdated
@cloutiertyler
cloutiertyler marked this pull request as ready for review September 9, 2026 03:20
Comment thread docs/docs/00200-core-concepts/00100-databases/00700-environment-variables.md Outdated
Comment thread docs/docs/00200-core-concepts/00100-databases/00700-environment-variables.md Outdated
Comment thread docs/docs/00200-core-concepts/00100-databases/00700-environment-variables.md Outdated
Comment thread docs/docs/00200-core-concepts/00100-databases/00700-environment-variables.md Outdated

@lisandroct lisandroct left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extremely minor change to handle some collisions with other methods in C# (besides the one that are already being handled).

Comment thread crates/bindings-csharp/Codegen.Tests/EnvironmentTests.cs
Comment thread crates/bindings-csharp/Codegen/Environment.cs Outdated

@JasonAtClockwork JasonAtClockwork left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran through ~8 different test cases end to end with C++ and only HandlerContext was missing (which I've added), other than the Windows spacetime publish issue everything worked.

Comment thread crates/cli/src/schema_extract.rs Outdated
Comment on lines +117 to +128
let mut child = tokio::process::Command::new(extractor)
.arg("extract-schema")
.arg(&module)
.arg("--host-type")
.arg(host_type.to_ascii_lowercase())
.env_clear()
.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::null())
.kill_on_drop(true)
.spawn()
.context("Cannot start local module schema inspection")?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let mut child = tokio::process::Command::new(extractor)
.arg("extract-schema")
.arg(&module)
.arg("--host-type")
.arg(host_type.to_ascii_lowercase())
.env_clear()
.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::null())
.kill_on_drop(true)
.spawn()
.context("Cannot start local module schema inspection")?;
let mut command = tokio::process::Command::new(extractor);
command
.arg("extract-schema")
.arg(&module)
.arg("--host-type")
.arg(host_type.to_ascii_lowercase())
.env_clear()
.stdin(Stdio::null())
.stdout(Stdio::piped())
.stderr(Stdio::null())
.kill_on_drop(true);
// Windows uses PATH when resolving dependent DLLs for the child
// process. Preserve only PATH while keeping the extractor's
// environment otherwise isolated.
#[cfg(windows)]
if let Some(path) = std::env::var_os("PATH") {
command.env("PATH", path);
}
let mut child = command
.spawn()
.context("Cannot start local module schema inspection")?;

During my testing spacetimedb-cli publish would fail because .env_clear() removed PATH.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied in e98ef53: schema inspection preserves PATH on Windows while clearing other inherited variables. Added a subprocess regression for environment isolation and the Windows PATH configuration. The five focused schema-extraction tests and strict CLI Clippy pass locally on macOS; Windows publishing verification is pending CI. Your HandlerContext change is included.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants