Implement persistent typed 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.
| #[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.
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).
JasonAtClockwork
left a comment
There was a problem hiding this comment.
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.
6972d73 to
b7adb97
Compare
|
|
||
| // Timeline references include historical links and links to other layers of | ||
| // a PR stack. A unique shared branch name identifies the companion PR; the | ||
| // exact public-submodule SHA is still checked before selecting its CI run. |
There was a problem hiding this comment.
I don't think that these changes are the right way to disambiguate private PR references. Moreover, this comment is misleading - this PR (5887) wasn't mentioned by multiple private PRs due to them being stacked; it were just.. directly mentioned multiple times, presumably by AI. Although the mentions have (mostly) been removed, they remain in the timeline history.
I think the correct disambiguation begins with searching the PR description of the private PR for an active mention of the public PR.
In this case, that would unfortunately still leave us with ambiguity, because it's still actively mentioned by both 3940 and 3157. To address that, we could either:
- Look for a particular keyword, such as
integrates #123 - Add a special section of the PR body for
Integrates public PR(s) - Remove the mention of the public PR from 3157, since I'm not sure how helpful it is to anyone at this point
My preference would be to do the third thing for now (as well as updating the logic to look in the PR description for active mentions), and then to add the special PR body section "eventually". It's been on my list for a while, but has never been a pressing priority.
There was a problem hiding this comment.
I made the "make sure the description matches" changes here: #5933. I may be OOO by the time you see this comment, but that PR LGTM if it looks good to you. Then we would just have to pick one of the follow-up strategies above (again, imo we should just remove the mention from 3157 and move on)
There was a problem hiding this comment.
I've merged that PR into this one, so I think we can revert the changes in this file (and remove the mention from that old PR) and then my code-owner review should no longer be a blocker!
Description of Changes
Implements typed environment variables, with declared schemas and typed module accessors. Publishing preserves stored values;
--env-onlyupdates them without a module bundle. Supports undeclared values, explicit deletion, and--replace-env, with atomic validation against the module's declarations. ENV edits use module-publishing permissions. Includes a 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.
--delete-dataclears environment values too.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. Covers persistence, undeclared values, environment-only updates, deletion/replacement, authorization, and stale module versions.