Joseph Doherty
051a59845d
- New FOLLOWUP-13: the DriverInstance.ResilienceConfig column was authored/persisted and already in the artifact; only the driver-node read path dropped it. Read-path plumbing + invalidate-on-change + respawn-on-change; deterministic verification. - FOLLOWUP-10 sub-gap (a) 'tier-defaults only' → RESOLVED by #13 (sub-gap (b) tracker reader still open). - STATUS.md: task #13 → completed; crit13 branch topology row; Completed-table row; findings #10 bullet updated; FOLLOWUP-13 linked.
64 lines
4.6 KiB
Markdown
64 lines
4.6 KiB
Markdown
# Follow-up #13 — Plumb per-instance ResilienceConfig through the deploy artifact
|
|
|
|
> **Status:** ✅ IMPLEMENTED (`fix/archreview-crit13-resilience-config-artifact` `75403caa`, off the crit10 branch)
|
|
> · **Surfaced by:** task #10's residual sub-finding (the invoker got tier defaults only) · **Task:** TaskCreate
|
|
> #13 · **Severity:** Medium (a silently-ignored authored config — the review's "unit-green ≠ wired" class) ·
|
|
> **Effort:** M. See [`STATUS.md`](STATUS.md) + [`FOLLOWUP-10`](FOLLOWUP-10-resilience-dispatch-gap.md).
|
|
|
|
## The finding
|
|
|
|
Task #10 wired the resilience pipeline into dispatch but the invoker only ever got the driver type's **tier
|
|
defaults** — `DriverCapabilityInvokerFactory.Create` passed `resilienceConfigJson: null`. The per-instance
|
|
`DriverInstance.ResilienceConfig` JSON column was therefore **silently ignored at runtime**.
|
|
|
|
**Key discovery:** the *write* side was already complete — the AdminUI `DriverResilienceSection` authors it, every
|
|
driver page persists it to the entity (create + update), and `ConfigComposer.SnapshotAndFlattenAsync` serializes
|
|
the **whole** `DriverInstance` entity, so `ResilienceConfig` was **already in the artifact JSON**. Only the driver
|
|
node's *read* path dropped it: `DriverInstanceSpec` didn't carry the column and the factory hard-coded null. So
|
|
#13 is pure read-path plumbing — a genuine dead-config-to-live fix, not a new feature.
|
|
|
|
## What shipped
|
|
|
|
- **`DriverInstanceSpec`** gains `string? ResilienceConfig`; **`DeploymentArtifact.TryReadSpec`** reads the column
|
|
(lenient `ReadString`, like the other fields).
|
|
- **`IDriverCapabilityInvokerFactory.Create`** takes `string? resilienceConfigJson`; the concrete factory parses
|
|
it via `DriverResilienceOptionsParser.ParseOrDefaults` (layering onto the tier), logs any parse diagnostic
|
|
(malformed JSON / unknown capability / misapplied Tier-C recycle) as a warning, and **never throws** — a bad
|
|
config degrades to tier defaults. `NullDriverCapabilityInvokerFactory` ignores the arg. Host DI passes a logger.
|
|
- **`DriverHostActor.SpawnChild`** threads `spec.ResilienceConfig` into `Create`.
|
|
- **Invalidate-on-change.** The pipeline cache keys on `(instance, host, capability)` and **ignores options on a
|
|
cache hit**, so a fresh invoker with changed options would otherwise keep serving the stale pipeline. `Create`
|
|
now calls `DriverResiliencePipelineBuilder.Invalidate(driverInstanceId)` first (no-op on first spawn; drops the
|
|
stale pipelines on a respawn). Invalidation is per-instance — a sibling's warm pipeline survives.
|
|
- **`DriverSpawnPlanner`** treats a `ResilienceConfig` change (incl. `null → json`) as a **stop + respawn** (the
|
|
invoker + its resolved options are bound to the child at spawn — the only way a change takes effect is to rebuild
|
|
the child). A **pure `DriverConfig` change stays an in-place delta** (no reconnect) — the resilience pipeline is
|
|
untouched. Snapshot (`DriverChildSnapshot`) carries `ResilienceConfig` for the diff.
|
|
|
|
## Design note — respawn vs. in-place swap
|
|
|
|
A ResilienceConfig-only change **reconnects the driver** (respawn). This is the simplest correct wiring: the
|
|
invoker/options are immutable for a child's lifetime (a clean invariant) and it reuses the existing stop+spawn
|
|
machinery. The alternative — an in-place invoker swap via a new actor message + mutable field — avoids the
|
|
reconnect but is materially more code for a rare operator-tuning op. Documented as a possible future optimization;
|
|
respawn is the v1.
|
|
|
|
## Verification (deterministic)
|
|
|
|
- **Override reaches execution** (`DriverCapabilityInvokerFactoryTests`): a `Read → retryCount:0` override
|
|
suppresses the tier-A Read retry (1 attempt); a **control** with no config retries (>1) — proving the single
|
|
attempt is the override, not a fluke.
|
|
- **Invalidate-on-create**: re-creating an instance's invoker drops its cached pipelines (count → 0); a sibling
|
|
instance's pipeline survives (scoped).
|
|
- **Malformed config** logs a warning + still yields a working (tier-default) invoker.
|
|
- **Planner** (`DriverSpawnPlannerTests`): ResilienceConfig change → respawn (incl. `null → json`); pure
|
|
DriverConfig change with unchanged resilience → delta.
|
|
- **Artifact** (`DeploymentArtifactTests`): the column is carried onto the spec / omitted → null.
|
|
- Core.Tests 243 (+5), Runtime.Tests 363 (+5), Host builds clean (0 warnings — dispatch sites untouched).
|
|
|
|
## Residual
|
|
|
|
None functional. A live rig check would fold into task #12 (the #10 live gate) — deploy a driver with a non-default
|
|
ResilienceConfig and confirm the observed retry/breaker behavior matches the override — but the deterministic tests
|
|
already prove the override reaches execution.
|