spec(db): verify ServiceSpec probe/sockets/require_ready round-trip through spec_json — add regression test + doc contract #90

New issue

Closed

opened 2026-05-01 05:14:49 +00:00 by despiegk · 0 comments

despiegk commented

2026-05-01 05:14:49 +00:00

Owner

What this checks

Before this work was committed, a concern was raised: do the three new ServiceSpec fields (probe, sockets, require_ready) require a SQLite schema migration?

The check passed: no migration is needed.

Why no migration is needed

ServiceSpec is persisted as a single JSON blob in the spec_json TEXT NOT NULL column of the services table. The schema has not changed — the column was there before; only the set of keys the JSON blob may contain has grown.

New fields and their serde defaults

Field	Type	serde default	Absent-key behaviour
`probe`	`Option<ServiceProbe>`	`#[serde(default, skip_serializing_if = "Option::is_none")]`	Deserialises as `None`; absent when `None` on write
`sockets`	`Vec<String>`	`#[serde(default, skip_serializing_if = "Vec::is_empty")]`	Deserialises as `[]`; absent when empty on write
`require_ready`	`bool`	`#[serde(default)]`	Deserialises as `false`; always written

All three use serde's standard absent-key → default-value rules. A spec_json blob written by any older version of the code (which lacks these keys entirely) deserialises cleanly with probe = None, sockets = [], require_ready = false.

Round-trip invariant

A spec_json blob written by a newer version of the code:

omits probe when None — identical to the old format
omits sockets when empty — identical to the old format
includes require_ready: false for services that never set it — older code would reject this as an unknown field only if #[serde(deny_unknown_fields)] were present; it is not present on ServiceSpec

So old code reading new blobs also survives.

Relevant code locations

Schema DDL: crates/hero_proc_lib/src/db/service/model.rs:148 — spec_json TEXT NOT NULL, unchanged
ServiceSpec struct and new fields: model.rs:219–254
ServiceProbe and ServiceProbeKind: model.rs:257–295
Supervisor evaluation: crates/hero_proc_server/src/supervisor/service_state.rs
SDK readiness helper (declare_ready / clear_ready): crates/hero_proc_sdk/src/ready.rs

Spec: what "round-trips correctly" means (acceptance criteria)

Old blob → new code: a services row written before these fields existed deserialises without error; probe is None, sockets is [], require_ready is false. The supervisor treats the service as having no probe, no socket invariant to check, and no readiness requirement. Behaviour is identical to pre-change.
New blob (opted-in service) → new code: a service that sets probe, sockets, and require_ready: true serialises all three keys into spec_json and deserialises them back with the exact same values.
New blob (not opted-in) → new code: a service that never sets any of the three fields produces a spec_json blob with no probe key, no sockets key, and require_ready: false. On next read the struct matches the in-memory representation before the write.
No ALTER TABLE needed: confirmed by code inspection — the spec_json column predates this change and no DDL migration was added.
Unknown-field tolerance (forward compat): ServiceSpec does not carry #[serde(deny_unknown_fields)], so if further fields are added in the future the same argument holds.

What this issue is NOT asking for

This is not asking for a new migration mechanism, a versioned schema, or any code change. It is asking for:

A regression test that proves the old-blob round-trip (first acceptance criterion above) — a unit test in model.rs that constructs a spec_json string that omits probe, sockets, and require_ready, deserialises it, and asserts the defaults. This prevents a future #[serde(deny_unknown_fields)] annotation or a struct rename from silently breaking the invariant.
A comment or doc-string on ServiceSpec (or next to the spec_json column DDL) that makes the intentional absent-key contract explicit, so future contributors know they must preserve it.

References

Parent reliability roadmap: #86
Issues that introduced the three fields: #83 (probe), #84 (require_ready), #78 (sockets)
Persistence layer: crates/hero_proc_lib/src/db/service/model.rs

## What this checks Before this work was committed, a concern was raised: do the three new `ServiceSpec` fields (`probe`, `sockets`, `require_ready`) require a SQLite schema migration? The check passed: **no migration is needed.** --- ## Why no migration is needed `ServiceSpec` is persisted as a single JSON blob in the `spec_json TEXT NOT NULL` column of the `services` table. The schema has not changed — the column was there before; only the set of keys the JSON blob may contain has grown. ### New fields and their serde defaults | Field | Type | serde default | Absent-key behaviour | |---|---|---|---| | `probe` | `Option<ServiceProbe>` | `#[serde(default, skip_serializing_if = "Option::is_none")]` | Deserialises as `None`; absent when `None` on write | | `sockets` | `Vec<String>` | `#[serde(default, skip_serializing_if = "Vec::is_empty")]` | Deserialises as `[]`; absent when empty on write | | `require_ready` | `bool` | `#[serde(default)]` | Deserialises as `false`; always written | All three use serde's standard absent-key → default-value rules. A `spec_json` blob written by any **older** version of the code (which lacks these keys entirely) deserialises cleanly with `probe = None`, `sockets = []`, `require_ready = false`. ### Round-trip invariant A `spec_json` blob written by a **newer** version of the code: - omits `probe` when `None` — identical to the old format - omits `sockets` when empty — identical to the old format - includes `require_ready: false` for services that never set it — older code would reject this as an unknown field **only if** `#[serde(deny_unknown_fields)]` were present; it is **not** present on `ServiceSpec` So old code reading new blobs also survives. ### Relevant code locations - Schema DDL: `crates/hero_proc_lib/src/db/service/model.rs:148` — `spec_json TEXT NOT NULL`, unchanged - `ServiceSpec` struct and new fields: `model.rs:219–254` - `ServiceProbe` and `ServiceProbeKind`: `model.rs:257–295` - Supervisor evaluation: `crates/hero_proc_server/src/supervisor/service_state.rs` - SDK readiness helper (`declare_ready` / `clear_ready`): `crates/hero_proc_sdk/src/ready.rs` --- ## Spec: what "round-trips correctly" means (acceptance criteria) - [ ] **Old blob → new code**: a `services` row written before these fields existed deserialises without error; `probe` is `None`, `sockets` is `[]`, `require_ready` is `false`. The supervisor treats the service as having no probe, no socket invariant to check, and no readiness requirement. Behaviour is identical to pre-change. - [ ] **New blob (opted-in service) → new code**: a service that sets `probe`, `sockets`, and `require_ready: true` serialises all three keys into `spec_json` and deserialises them back with the exact same values. - [ ] **New blob (not opted-in) → new code**: a service that never sets any of the three fields produces a `spec_json` blob with no `probe` key, no `sockets` key, and `require_ready: false`. On next read the struct matches the in-memory representation before the write. - [ ] **No `ALTER TABLE` needed**: confirmed by code inspection — the `spec_json` column predates this change and no DDL migration was added. - [ ] **Unknown-field tolerance (forward compat)**: `ServiceSpec` does not carry `#[serde(deny_unknown_fields)]`, so if further fields are added in the future the same argument holds. --- ## What this issue is NOT asking for This is not asking for a new migration mechanism, a versioned schema, or any code change. It is asking for: 1. A **regression test** that proves the old-blob round-trip (first acceptance criterion above) — a unit test in `model.rs` that constructs a `spec_json` string that omits `probe`, `sockets`, and `require_ready`, deserialises it, and asserts the defaults. This prevents a future `#[serde(deny_unknown_fields)]` annotation or a struct rename from silently breaking the invariant. 2. A **comment or doc-string** on `ServiceSpec` (or next to the `spec_json` column DDL) that makes the intentional absent-key contract explicit, so future contributors know they must preserve it. --- ## References - Parent reliability roadmap: https://forge.ourworld.tf/lhumina_code/hero_proc/issues/86 - Issues that introduced the three fields: #83 (probe), #84 (require_ready), #78 (sockets) - Persistence layer: `crates/hero_proc_lib/src/db/service/model.rs`