refactor(sablier): stage 1 of the InstanceInfo split - typed InstanceConfig with one parser and one enabled-semantics#1023
Merged
Conversation
… god struct InstanceInfo is four things at once: provider-reported state, a parsed-label configuration bag, the API wire format, and the persisted session record. This is the first stage of pulling those apart, scoped to the configuration concern and deliberately wire-compatible. All sablier.* label parsing now lives in one function, InstanceConfigFromLabels, producing a typed InstanceConfig (Enabled is a bool, durations are durations, window values are validated). It takes a logger instead of using the global slog, restoring the instance context on warnings. PopulateEnabledAndGroup becomes a thin adapter that providers keep calling: it stores the typed config under the new additive `config` field and mirrors it into the flat legacy fields, which stay byte-identical for API consumers and persisted session records (pinned by a wire-compat test; every pre-existing PopulateEnabledAndGroup test passes unchanged). The scattered `Enabled == "true"` string comparisons (six call sites across autostop, autowarm, expiration, request handling and the running-hours watcher) collapse into IsEnabled() accessors on InstanceInfo and InstanceConfiguration. The InstanceInfo accessor prefers the parsed config and falls back to the flat field for records persisted by older versions. Next stages (separate PRs): an API DTO so the wire format stops being the domain struct, and a versioned session record so the store stops persisting label config.
github-actions
Bot
added
the
documentation
Test Results✅ All tests passed! | 779 tests in 89.585s |
|
The provider inspect integration tests compare the full InstanceInfo with DeepEqual; they now fail because inspect results carry the new additive Config field. Since PopulateEnabledAndGroup mirrors the parsed config 1:1 into the flat fields each test case already declares, the expectation is derived from those fields at the existing per-case fixup block instead of hand-writing ~70 Config literals. This also keeps working for the Kubernetes cases that configure sablier via annotations. Verified against the real runtimes locally: docker (dind), swarm (dind), kubernetes (k3s, including the annotation cases) and podman (pind) inspect suites all pass.
Member
Author
|
CI failures fixed: the provider inspect suites DeepEqual the full InstanceInfo and needed the new additive |
|
acouvreur
added a commit
that referenced
this pull request
Jul 11, 2026
…truct (#1024) The blocking endpoint answered with an ad-hoc map{"session": sessionState} whose bytes were shaped by a custom SessionState.MarshalJSON, while the OpenAPI schema was generated from the struct tags. The two had drifted apart from day one: the published schema said `instances` was a map of name to entry and had no `status` property, while the wire carried an ARRAY of {instance, error} entries plus `status`. Anyone building a reverse-proxy plugin against the published contract parsed the wrong shape. The SessionResponse/ThemesResponse types existed only to feed the annotations; nothing kept them in sync with what handlers actually wrote. The handlers now build and return the annotated DTOs (NewSessionResponse, ThemesResponse), so the generated schema is derived from the same types that produce the bytes and cannot drift again. The wire shape is unchanged except that instance entries are now sorted by name instead of map-iteration random (ordering was never part of the contract). SessionState loses its MarshalJSON: the domain type no longer carries wire concerns, which is stage 2 of the InstanceInfo/session split (stage 1: #1023). Its marshalling tests move to the API package against the DTO, joined by a wire-shape pin so the array+status contract is enforced by test rather than by accident.
acouvreur
added a commit
that referenced
this pull request
Jul 11, 2026
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.
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
InstanceInfo(instance.go) is four things at once:Status,CurrentReplicas,DesiredReplicas(plusReadyAt, whose own comment admits it "is set internally by Sablier and is never populated by a provider").Enabledis a string compared against"true"in six scattered places (autostop, autowarm, expiration, request handling, running-hours watcher, and the parser itself);RunningHours/RunningDaysare raw strings;ReadyAfter,ReadyOnStart,AntiAffinity,ScaleConfigride along.Every feature adds a field here and touches all four concerns at once; nobody can tell which fields a provider must set, which Sablier owns, and which are configuration. Label parsing also warned through the global
slog, bypassing the configured handler.Approach: staged, wire-compatible
The full split has two compatibility cliffs - the wire format (plugins) and the persisted records (upgrades with live state) - so it lands in stages. This PR is stage 1, scoped to the configuration concern, and changes zero existing bytes on the wire.
What this stage does
sablier.*label interpretation now lives inInstanceConfigFromLabels, producing a typedInstanceConfig(Enabled bool, realtime.Duration, validated window strings,Scale *ScaleConfig). It takes a logger (restoring per-instance context on warnings) instead of the globalslog.Enabled == "true"string comparisons collapse intoIsEnabled()accessors onInstanceInfoandInstanceConfiguration. TheInstanceInfoaccessor prefers the parsed config and falls back to the flat field for session records persisted by older versions.PopulateEnabledAndGroup(the function all five providers call) becomes a thin adapter: it stores the typed config under a new additiveconfigkey and mirrors it into the flat legacy fields byte-identically -enabledstays the raw label string the API has always exposed.Evidence it is behavior-preserving
PopulateEnabledAndGrouptest (ready-after, groups, running-hours, anti-affinity, scale labels, blkio labels) passes unchanged - they assert the flat fields.TestPopulateEnabledAndGroup_WireCompatpins the contract: legacy keys keep their exact names and raw values,configis strictly additive.configsession record and assertsIsEnabled()still answers correctly.InstanceConfigFromLabels(enable exact-match semantics, group gating, invalid-value warn-and-ignore, scale-presence condition).openapi.jsonregenerated (gains the additivesablier.InstanceConfigschema);make check-generate, full test suite andgolangci-lintpass.Next stages (separate PRs)
internal/apiso the wire format stops being the domain struct (pairs with the OpenAPI drift finding).InstanceInfo.ReadyAtand friends move off the provider-facing type).Docs
No documentation impact beyond the regenerated OpenAPI spec: no labels, config options, or behavior changed.