Context
PR #4114 fixed a P1 state-persistence correctness bug: the state-encryption key was resolved off the global ENGINE_STATE lock, but ENGINE_STATE already serializes the persisted-state writes, so key selection was out-of-order with the writes. A caller holding a key resolved before a provider key-rotation could win the last write after a concurrent caller already persisted the rotated key, tagging the single state file with the stale key-id. decode_encrypted_state_envelope accepts only the currently-resolved key-id, so restart rejects the file as a key-id mismatch and the persisted state is unreadable.
The fix resolves the key under the held ENGINE_STATE guard, at the persist site (persist_engine_state_to_storage), so key selection is in the same serialized order as the writes. This is correct, but it reverts the off-lock optimization #4114 originally introduced: for the command provider the KMS/HSM subprocess now runs under the guard during every actual persist.
Why this matters (it's the mandatory production path)
The state-key provider is selected per-operator via TBTC_SIGNER_STATE_KEY_PROVIDER (default env), but the production profile fail-closes the env provider and requires command/KMS (resolve_state_key_provider_plan + signer_profile_is_production). So in production the command provider is effectively mandatory, and the under-guard KMS subprocess runs on every production signer's every persist (signing rounds, DKG, lifecycle). A slow KMS serializes engine throughput at roughly one KMS round-trip at a time; a hung command is bounded by terminate_state_key_command, so it's a bounded head-of-line stall, not a deadlock. (The env provider — dev/test only — has no subprocess and no regression.)
Goal
Recover off-lock (or per-persist-subprocess-free) state-key resolution for the command provider without reintroducing the stale-key write-ordering race.
Gate on measurement first
The ENGINE_STATE guard already holds the persist fsync I/O. A KMS command in the low-ms range is comparable noise, and persist rate is bounded by signing throughput. Measure KMS-command latency × concurrent-persist rate against the existing fsync cost before investing — the regression may be acceptable in practice.
Options (each has a real, non-free tradeoff)
- A — cheap "current key-id" check from the provider. Resolve the full key off-lock; under the guard, cheaply check the current key-id; use the off-lock key if unchanged, else re-resolve under the guard (rare). Recovers off-lock for the common no-rotation case and preserves the KMS intent (key resolved only when needed). Cost: a provider-contract addition — the key command must expose key-id cheaply — and likely an envelope key-id scheme change (KMS native version-id vs the current local hash-of-key).
- B′ — cache the resolved key for the process lifetime. Eliminates the per-persist subprocess entirely; correct if live (silent, no-restart) rotation is declared unsupported. Cost: increases key residency in process memory, which directly undercuts why an operator chose KMS (source fresh, zeroize after).
- C — separate persist-linearization lock held across resolve+write. Keeps the subprocess off
ENGINE_STATE so non-persisting reads aren't blocked. Cost: persisting throughput still serializes through the KMS; only a partial recovery; more invasive (requires a state snapshot under ENGINE_STATE).
Decode-side key history (store old keys) is rejected — it defeats the point of KMS-sourced keys.
Recommendation
Measure first. If it matters, lean Option A (best preserves the KMS security intent), but the choice is security-laden (key residency, key-id scheme) and should be made with the signer/KMS owners. Do not speculatively implement.
Invariants any solution must preserve (from the #4114 fix + review)
- Idempotent replays / pre-persist rejections return before any key resolution (a key-provider outage must not fail a non-persisting call).
- Durable-marker sites resolve the key before the marker insert; marker-rollback-on-persist-failure stays intact.
- The final write to the single state file is tagged with the key that will be current at restart-decode time.
Follow-up to #4114 (Codex P1 review).
Context
PR #4114 fixed a P1 state-persistence correctness bug: the state-encryption key was resolved off the global
ENGINE_STATElock, butENGINE_STATEalready serializes the persisted-state writes, so key selection was out-of-order with the writes. A caller holding a key resolved before a provider key-rotation could win the last write after a concurrent caller already persisted the rotated key, tagging the single state file with the stale key-id.decode_encrypted_state_envelopeaccepts only the currently-resolved key-id, so restart rejects the file as a key-id mismatch and the persisted state is unreadable.The fix resolves the key under the held
ENGINE_STATEguard, at the persist site (persist_engine_state_to_storage), so key selection is in the same serialized order as the writes. This is correct, but it reverts the off-lock optimization #4114 originally introduced: for thecommandprovider the KMS/HSM subprocess now runs under the guard during every actual persist.Why this matters (it's the mandatory production path)
The state-key provider is selected per-operator via
TBTC_SIGNER_STATE_KEY_PROVIDER(defaultenv), but theproductionprofile fail-closes theenvprovider and requirescommand/KMS (resolve_state_key_provider_plan+signer_profile_is_production). So in production thecommandprovider is effectively mandatory, and the under-guard KMS subprocess runs on every production signer's every persist (signing rounds, DKG, lifecycle). A slow KMS serializes engine throughput at roughly one KMS round-trip at a time; a hung command is bounded byterminate_state_key_command, so it's a bounded head-of-line stall, not a deadlock. (Theenvprovider — dev/test only — has no subprocess and no regression.)Goal
Recover off-lock (or per-persist-subprocess-free) state-key resolution for the
commandprovider without reintroducing the stale-key write-ordering race.Gate on measurement first
The
ENGINE_STATEguard already holds the persistfsyncI/O. A KMS command in the low-ms range is comparable noise, and persist rate is bounded by signing throughput. Measure KMS-command latency × concurrent-persist rate against the existing fsync cost before investing — the regression may be acceptable in practice.Options (each has a real, non-free tradeoff)
ENGINE_STATEso non-persisting reads aren't blocked. Cost: persisting throughput still serializes through the KMS; only a partial recovery; more invasive (requires a state snapshot underENGINE_STATE).Decode-side key history (store old keys) is rejected — it defeats the point of KMS-sourced keys.
Recommendation
Measure first. If it matters, lean Option A (best preserves the KMS security intent), but the choice is security-laden (key residency, key-id scheme) and should be made with the signer/KMS owners. Do not speculatively implement.
Invariants any solution must preserve (from the #4114 fix + review)
Follow-up to #4114 (Codex P1 review).