refactor(store): stage 3 of the InstanceInfo split - versioned session records with transparent legacy upgrade#1025
Merged
Merged
Conversation
Both stores serialized the raw domain InstanceInfo as the persisted session
entry, making the storage schema "whatever the domain struct happens to
marshal as": every field added to InstanceInfo silently became persisted
state with no version to gate evolution and no way to recognize records
written by an older release.
Sessions are now wrapped in a SessionRecord envelope ({"v":1,"instance":
{...}}) owned next to the Store interface. Its unmarshaler transparently
upgrades pre-versioning payloads (bare InstanceInfo documents), so live
sessions in valkey and state.json snapshots survive the upgrade instead of
being dropped. Foreign keys sharing the valkey keyspace keep the previous
tolerance: they decode to a record with an empty instance name and are
skipped by the existing name check.
The Store interface is unchanged (Get/Put still speak InstanceInfo), so the
core and providers are untouched; this is stage 3 of the InstanceInfo split
(stage 1: #1023, stage 2: #1024) and the prerequisite for slimming what
sessions actually persist, which will be a schema-version bump instead of a
silent break.
Migration coverage: record round-trip and legacy-upgrade unit tests, an
inmemory legacy-snapshot load test, and valkey testcontainers tests reading
a legacy payload through Get and Range and asserting new entries carry the
version. The sabliercmd storage fixtures already use the legacy shape, so
the state-file load tests now exercise the upgrade end to end.
Test Results✅ All tests passed! | 794 tests in 102.321s |
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Finding
Stage 3 of the
InstanceInfosplit (stage 1: #1023, stage 2: #1024). This one targets the persistence facet: both stores serialized the raw domainInstanceInfoas the session entry, so the storage schema was "whatever the domain struct happens to marshal as". Consequences:InstanceInfosilently became persisted state.instanceRequestpath even warns about invalid stored running-hours values).A constraint discovered during design (worth knowing)
The stored record is not just bookkeeping: on the ready path,
instanceRequestserves the storedInstanceInfoback to callers without any provider call - that is Sablier's documented warm-session performance model. So the record cannot be slimmed down to "just session fields" without either changing API responses or re-inspecting per request. This stage therefore delivers the versioned envelope + migration machinery; actually removing label config from the record is a later stage that will now be an explicitv2schema bump instead of a silent break.What this does
SessionRecordenvelope -{"v":1,"instance":{...}}- defined next to theStoreinterface.SessionRecord.UnmarshalJSONtransparently upgrades pre-versioning payloads (bareInstanceInfodocuments): live sessions in valkey andstate.jsonsnapshots survive the Sablier upgrade instead of being dropped or mis-read.Storeinterface is unchanged (Get/Putstill speakInstanceInfo), so core, providers, API and wire are untouched - zero behavior change outside the persisted bytes.Migration evidence
TestSessionRecordLegacyUpgrade- a bare pre-versioning payload decodes to a current-version record with every field intact.TestInMemorySnapshotIsVersioned- new snapshots carry"v":1; a legacy-shaped snapshot loads and upgrades.TestValKeySessionRecordMigration(testcontainers) - a legacy payload written directly into valkey is readable through bothGetandRange(the metrics path), and new entries carry the version.sabliercmdstorage fixtures already used the legacy shape, so the existing state-file load tests now exercise the upgrade end to end - unchanged and passing.Full
pkg/sablier+ store suites (including valkey via testcontainers) andgolangci-lintpass.Docs
No user-facing impact: no config, labels, API, or generated docs change (the record is persistence-internal and does not appear in the OpenAPI schema).