Developer
87f42b4ec3
Correctness fix for multi-instance: 6 hooks gain optional serviceId in queryKey; 7 API fns append ?service_id; 4 backup endpoints gain service_id filter (Alertmanager/Prometheus status already take it — zero backend change there); 3 tabs pass instance.id; dashboard widgets unaffected; all params optional (backward-compat). usePrometheusTargets + useMonitoringMachines stay global.
304 lines
16 KiB
Markdown
304 lines
16 KiB
Markdown
# SDD Spec: Per-Instance Hook Scoping
|
||
|
||
**Change:** `per-instance-hook-scoping`
|
||
**Phase:** spec
|
||
**Date:** 2026-07-09
|
||
|
||
This is a **correctness fix**, not a feature. When multiple instances of Alertmanager, Prometheus, or the backups service are configured, the service-type tabs (`AlertsTab`, `MetricsTab`, `JobsTab`) show data for whichever instance resolves first globally — not the instance whose page the operator is viewing. This spec defines the acceptance requirements to scope the observability and backup hooks to the instance the operator selected. Each requirement is testable. Requirements derived from `proposal.md` §6 (success criteria) and the resolved §8 (no question round needed).
|
||
|
||
## Requirement categories
|
||
|
||
1. Frontend hooks (serviceId param + queryKey)
|
||
2. API client functions (serviceId → query param)
|
||
3. Backend backup endpoints (service_id filter)
|
||
4. Tabs pass instance.id (TODO comments removed)
|
||
5. Non-regression (dashboard widgets, global hooks untouched)
|
||
6. Backward compatibility
|
||
7. Test + build greenness
|
||
|
||
---
|
||
|
||
## 1. Frontend hooks (serviceId param + queryKey)
|
||
|
||
### Requirement: PI-101 — useAlertmanagerAlerts accepts serviceId
|
||
|
||
The hook `useAlertmanagerAlerts` MUST accept an optional `serviceId?: string` argument. When provided, the hook MUST (a) include the `serviceId` in its TanStack Query `queryKey` (so caches do not collide across instances), and (b) pass the `serviceId` to its underlying fetch function. When omitted, the hook MUST behave identically to today (global/first-configured resolution).
|
||
|
||
#### Scenario: per-instance cache key
|
||
|
||
- GIVEN two configured Alertmanager instances A and B
|
||
- WHEN `useAlertmanagerAlerts("A")` and `useAlertmanagerAlerts("B")` are both mounted
|
||
- THEN their `queryKey`s MUST differ (the `serviceId` appears in the key)
|
||
- AND a cache hit for A MUST NOT be served for B (no stale cross-instance data)
|
||
|
||
#### Scenario: omitted serviceId is backward-compatible
|
||
|
||
- GIVEN a caller that does not pass `serviceId`
|
||
- WHEN the hook runs
|
||
- THEN its behavior MUST match the pre-change hook (queryKey shape equivalent, fetch path identical)
|
||
|
||
### Requirement: PI-102 — useAlertmanagerStatus accepts serviceId
|
||
|
||
The hook `useAlertmanagerStatus` MUST accept an optional `serviceId?: string`, include it in its `queryKey`, and pass it to its fetch function. Omitting it MUST preserve today's behavior.
|
||
|
||
#### Scenario: per-instance cache key
|
||
|
||
- GIVEN instances A and B
|
||
- WHEN `useAlertmanagerStatus("A")` and `useAlertmanagerStatus("B")` are mounted
|
||
- THEN the `queryKey`s MUST differ and a cache hit for A MUST NOT serve B
|
||
|
||
### Requirement: PI-103 — usePrometheusStatus accepts serviceId
|
||
|
||
The hook `usePrometheusStatus` MUST accept an optional `serviceId?: string`, include it in its `queryKey`, and pass it to its fetch function. Omitting it MUST preserve today's behavior.
|
||
|
||
#### Scenario: per-instance cache key
|
||
|
||
- GIVEN instances A and B
|
||
- WHEN `usePrometheusStatus("A")` and `usePrometheusStatus("B")` are mounted
|
||
- THEN the `queryKey`s MUST differ and a cache hit for A MUST NOT serve B
|
||
|
||
### Requirement: PI-104 — useBackupJobs accepts serviceId
|
||
|
||
The hook `useBackupJobs` MUST accept an optional `serviceId?: string`, include it in its `queryKey`, and pass it to its fetch function. Omitting it MUST preserve today's behavior.
|
||
|
||
#### Scenario: per-instance cache key
|
||
|
||
- GIVEN backup services A and B
|
||
- WHEN `useBackupJobs("A")` and `useBackupJobs("B")` are mounted
|
||
- THEN the `queryKey`s MUST differ and a cache hit for A MUST NOT serve B
|
||
|
||
### Requirement: PI-105 — useBackupRuns accepts serviceId
|
||
|
||
The hook `useBackupRuns(jobId, status, serviceId?)` MUST accept an optional `serviceId?: string` in addition to its existing `jobId` and `status` arguments, include it in its `queryKey`, and pass it to its fetch function. Omitting it MUST preserve today's behavior.
|
||
|
||
#### Scenario: per-instance cache key
|
||
|
||
- GIVEN backup services A and B
|
||
- WHEN `useBackupRuns("job1", "all", "A")` and `useBackupRuns("job1", "all", "B")` are mounted
|
||
- THEN the `queryKey`s MUST differ
|
||
|
||
### Requirement: PI-106 — useBackupAlerts accepts serviceId
|
||
|
||
The hook `useBackupAlerts(jobId, acknowledged, severity, serviceId?)` MUST accept an optional `serviceId?: string` in addition to its existing arguments, include it in its `queryKey`, and pass it to its fetch function. Omitting it MUST preserve today's behavior.
|
||
|
||
#### Scenario: per-instance cache key
|
||
|
||
- GIVEN backup services A and B
|
||
- WHEN `useBackupAlerts("job1", false, "all", "A")` and `useBackupAlerts("job1", false, "all", "B")` are mounted
|
||
- THEN the `queryKey`s MUST differ
|
||
|
||
### Requirement: PI-107 — usePrometheusTargets and useMonitoringMachines stay global
|
||
|
||
The hooks `usePrometheusTargets` and `useMonitoringMachines` MUST NOT gain a `serviceId` parameter.
|
||
|
||
- `usePrometheusTargets` returns Node Exporter scrape targets for *external* Prometheus instances via `http_sd_configs` (cross-instance by design).
|
||
- `useMonitoringMachines` is a global cross-service concept (not per-instance).
|
||
|
||
#### Scenario: global hooks unchanged
|
||
|
||
- GIVEN the change is applied
|
||
- WHEN `usePrometheusTargets()` and `useMonitoringMachines()` signatures are inspected
|
||
- THEN neither accepts a `serviceId` arg and both behave identically to before
|
||
|
||
---
|
||
|
||
## 2. API client functions (serviceId → query param)
|
||
|
||
### Requirement: PI-108 — Observability fetch functions pass service_id query param
|
||
|
||
The fetch functions `fetchAlertmanagerAlerts`, `fetchAlertmanagerStatus`, and `fetchPrometheusStatus` (in `api/client.ts`) MUST each accept an optional `serviceId?: string`. When provided, the function MUST append `service_id=<id>` to the request URL as a query parameter (correctly using `?` for the first param and `&` for subsequent ones). When omitted, the request MUST be identical to today (no `service_id` param sent).
|
||
|
||
#### Scenario: serviceId appended
|
||
|
||
- GIVEN `fetchPrometheusStatus("svc-abc")` is called
|
||
- WHEN the request is issued
|
||
- THEN the URL contains `service_id=svc-abc`
|
||
|
||
#### Scenario: omitted serviceId is a no-op
|
||
|
||
- GIVEN `fetchPrometheusStatus()` is called (no serviceId)
|
||
- WHEN the request is issued
|
||
- THEN the URL MUST NOT contain `service_id`
|
||
|
||
### Requirement: PI-109 — Backup fetch functions pass service_id query param
|
||
|
||
The fetch functions `fetchBackupJobs`, `fetchBackupRuns`, `fetchBackupAlerts`, and `fetchBackupDashboard` (in `api/backups.ts`) MUST each accept an optional `serviceId?: string`. When provided, the function MUST append `service_id=<id>` to the request URL. When omitted, the request MUST be identical to today.
|
||
|
||
#### Scenario: serviceId appended alongside existing params
|
||
|
||
- GIVEN `fetchBackupRuns("job1", "failed", "svc-xyz")` is called
|
||
- WHEN the request is issued
|
||
- THEN the URL contains both `job_id=job1&status=failed` (existing params) AND `service_id=svc-xyz`
|
||
|
||
#### Scenario: omitted serviceId is a no-op
|
||
|
||
- GIVEN `fetchBackupJobs()` is called (no serviceId)
|
||
- WHEN the request is issued
|
||
- THEN the URL MUST NOT contain `service_id`
|
||
|
||
---
|
||
|
||
## 3. Backend backup endpoints (service_id filter)
|
||
|
||
### Requirement: PI-110 — Backup endpoints accept optional service_id
|
||
|
||
The endpoints `get_backup_jobs`, `get_backup_runs`, `get_backup_alerts`, and `get_backup_dashboard` in `routers/backups.py` MUST each accept an optional `service_id: str | None = None` query parameter and thread it into the corresponding `SettingsStore` query method.
|
||
|
||
#### Scenario: service_id passed through to store
|
||
|
||
- GIVEN a request `GET /api/backups/jobs?service_id=svc-xyz`
|
||
- WHEN the endpoint runs
|
||
- THEN `store.list_backup_jobs` MUST be called with `service_id="svc-xyz"`
|
||
|
||
### Requirement: PI-111 — SettingsStore backup queries filter by service_id when provided
|
||
|
||
The methods `list_backup_jobs`, `list_backup_runs`, and `list_backup_alerts` in `services/settings_store.py` MUST each accept an optional `service_id: str | None` parameter. When `service_id` is a non-null, non-empty string, the query MUST include a `WHERE service_id = ?` clause (or equivalent scoping) returning only rows attributed to that service. When `service_id` is `None` or empty, the query MUST return ALL rows (no filter) — preserving backward-compatibility for existing callers.
|
||
|
||
The `backup_jobs` and `backup_runs` tables already carry `service_id` attribution (from the `services-as-hub-ia` Slice 3 work); this requirement adds the filter, not the column.
|
||
|
||
#### Scenario: filtered to one service
|
||
|
||
- GIVEN the store holds jobs for services A and B
|
||
- WHEN `list_backup_jobs(service_id="A")` is called
|
||
- THEN only service A's jobs MUST be returned
|
||
|
||
#### Scenario: no filter returns all
|
||
|
||
- GIVEN the store holds jobs for services A and B
|
||
- WHEN `list_backup_jobs(service_id=None)` is called
|
||
- THEN jobs for BOTH services MUST be returned (backward-compat)
|
||
|
||
#### Scenario: empty string is treated as no filter
|
||
|
||
- GIVEN the store holds jobs for services A and B
|
||
- WHEN `list_backup_jobs(service_id="")` is called
|
||
- THEN jobs for BOTH services MUST be returned (defensive against falsy strings)
|
||
|
||
### Requirement: PI-112 — Alertmanager/Prometheus status endpoints already accept service_id
|
||
|
||
The endpoints `get_alertmanager_alerts`, `get_alertmanager_status`, and `get_prometheus_status` in `routers/monitoring.py` ALREADY accept a `service_id: str | None = None` query parameter and resolve via `resolve_service_record(store, "<type>", service_id)`. No backend change is required for these. This requirement exists to document the asymmetry: the frontend wiring change for these hooks is necessary (PI-108 + PI-101..103), but the backend endpoint work is zero.
|
||
|
||
#### Scenario: existing endpoint behavior confirmed
|
||
|
||
- GIVEN the change is applied
|
||
- WHEN `get_alertmanager_alerts(service_id="svc-a")` is called
|
||
- THEN it MUST resolve to the `svc-a` Alertmanager instance (pre-existing behavior, unchanged)
|
||
|
||
---
|
||
|
||
## 4. Tabs pass instance.id (TODO comments removed)
|
||
|
||
### Requirement: PI-113 — AlertsTab passes instance.id
|
||
|
||
The component `pages/service-tabs/AlertsTab.tsx` MUST call `useAlertmanagerAlerts(instance.id)` and `useAlertmanagerStatus(instance.id)` (passing the `instance.id` it already receives as a prop). The existing TODO comment about "hooks don't accept a service_id yet" MUST be removed.
|
||
|
||
#### Scenario: data is instance-scoped
|
||
|
||
- GIVEN two Alertmanager instances A and B are configured
|
||
- WHEN the operator navigates to instance A's service page
|
||
- THEN AlertsTab MUST show alerts sourced from instance A (not B, not first-resolved)
|
||
|
||
### Requirement: PI-114 — MetricsTab passes instance.id
|
||
|
||
The component `pages/service-tabs/MetricsTab.tsx` MUST call `usePrometheusStatus(instance.id)` (passing the `instance.id` it already receives as a prop). The existing TODO comment MUST be removed. (`usePrometheusTargets` stays global — PI-107.)
|
||
|
||
#### Scenario: data is instance-scoped
|
||
|
||
- GIVEN two Prometheus instances A and B are configured
|
||
- WHEN the operator navigates to instance A's service page
|
||
- THEN MetricsTab MUST show status for instance A (not B, not first-resolved)
|
||
|
||
### Requirement: PI-115 — JobsTab passes instance.id
|
||
|
||
The component `pages/service-tabs/JobsTab.tsx` MUST call `useBackupJobs(instance.id)`, `useBackupRuns(<jobId>, <status>, instance.id)`, and `useBackupAlerts(<jobId>, <acknowledged>, <severity>, instance.id)`. The existing TODO comment MUST be removed.
|
||
|
||
#### Scenario: data is instance-scoped
|
||
|
||
- GIVEN two backups service instances A and B are configured
|
||
- WHEN the operator navigates to instance A's service page
|
||
- THEN JobsTab MUST show jobs/runs/alerts for service A only
|
||
|
||
### Requirement: PI-116 — Instance switcher re-scopes tab data
|
||
|
||
The `ServicePage` instance switcher (present when `enabledSiblings.length > 1`) changes the active instance. When the operator switches from instance A to instance B, the three tabs MUST re-fetch and re-render with instance B's data. (This falls out naturally from passing `instance.id` into the hook `queryKey`s — switching the prop changes the key, triggering a fresh fetch — but it is verified explicitly because it's the user-visible correctness goal.)
|
||
|
||
#### Scenario: switch re-scopes
|
||
|
||
- GIVEN the operator is viewing instance A's MetricsTab (data from A)
|
||
- WHEN the operator uses the instance switcher to select instance B
|
||
- THEN the tab MUST re-render with instance B's status (not A's cached data)
|
||
|
||
---
|
||
|
||
## 5. Non-regression (dashboard widgets, global hooks untouched)
|
||
|
||
### Requirement: PI-117 — Dashboard widgets render identically before/after
|
||
|
||
Dashboard widgets (`AlertmanagerAlertsWidget`, `BackupsWidget`, `PrometheusMetricWidget`/`Metric*`, etc.) resolve their service via the widget-instance's `service_id` binding through `useWidgetData` — a different code path from the hooks changed here. They MUST render identically before and after this change. The widget component tests MUST remain unchanged and green.
|
||
|
||
#### Scenario: widget path unaffected
|
||
|
||
- GIVEN the change is applied
|
||
- WHEN a dashboard widget fetches its data
|
||
- THEN the fetch path (`useWidgetData(widget.id, …)`) MUST be unchanged; no `useAlertmanager*` / `useBackup*` hook is on the widget data path
|
||
|
||
---
|
||
|
||
## 6. Backward compatibility
|
||
|
||
### Requirement: PI-118 — All new params are optional; omitting equals today's behavior
|
||
|
||
Every new parameter added by this change (hook `serviceId?: string`, fetch-function `serviceId?: string`, endpoint `service_id: str | None = None`, store-method `service_id: str | None = None`) MUST be optional with a default that preserves today's behavior. No existing caller is forced to pass the new argument. Existing callers that omit it MUST see identical results to the pre-change code.
|
||
|
||
#### Scenario: existing callers unbroken
|
||
|
||
- GIVEN any pre-existing caller of `useBackupJobs()`, `fetchBackupRuns()`, `GET /api/backups/jobs`, or `store.list_backup_jobs()`
|
||
- WHEN the change is applied and the caller is run without modification
|
||
- THEN the behavior MUST be identical to before (no error, same data returned)
|
||
|
||
---
|
||
|
||
## 7. Test + build greenness
|
||
|
||
### Requirement: PI-119 — Backend tests + lint green
|
||
|
||
`PYTHONPATH=src python3 -m pytest -q` and `PYTHONPATH=src python3 -m ruff check src tests` from `backend/` MUST pass. New backend tests MUST cover: `list_backup_*` filtering by `service_id` (filtered, unfiltered, empty-string); backup endpoint threading `service_id` into the store.
|
||
|
||
#### Scenario: backup filter tests exist
|
||
|
||
- GIVEN the change is applied
|
||
- WHEN `pytest tests/test_backups.py` (or equivalent) runs
|
||
- THEN it MUST include a test asserting `service_id="A"` returns only A's rows AND `service_id=None` returns all rows
|
||
|
||
### Requirement: PI-120 — Frontend typechecks, builds, and lints
|
||
|
||
`npm run build` (which runs `tsc -b` + `vite build`) and `npm run lint` from `frontend/` MUST pass (0 errors; pre-existing warnings are acceptable).
|
||
|
||
### Requirement: PI-121 — Hook + tab tests cover the new serviceId wiring
|
||
|
||
The frontend test suite MUST include:
|
||
|
||
- A hook test asserting the `serviceId` appears in the `queryKey` (per-instance cache isolation) for at least one representative hook (e.g. `useBackupJobs`).
|
||
- A tab test asserting `instance.id` is passed through to the hook (for at least one of AlertsTab / MetricsTab / JobsTab).
|
||
|
||
#### Scenario: per-instance queryKey test exists
|
||
|
||
- GIVEN the change is applied
|
||
- WHEN the test suite runs
|
||
- THEN a test MUST assert that calling the hook with different `serviceId` values produces different `queryKey`s
|
||
|
||
#### Scenario: tab passes instance.id test exists
|
||
|
||
- GIVEN the change is applied
|
||
- WHEN the test suite runs
|
||
- THEN a test MUST assert that at least one tab component passes its received `instance.id` to the scoped hook
|
||
|
||
---
|
||
|
||
## Notes for downstream phases
|
||
|
||
- **Design (next phase)** should settle: the exact `queryKey` shape convention (e.g. `["backups", "jobs", serviceId ?? ""]` vs. appending the id only when present), whether a small shared helper for `appendServiceIdParam(url, serviceId)` is worth extracting in `api/shared.ts`, and whether `fetchBackupDashboard` (which feeds the dashboard widget via a different hook path) is in scope or deliberately excluded.
|
||
- **Tasks** should be a single slice (~250–350 lines, frontend-dominant) per the proposal; if the backend `SettingsStore` filter changes touch more than expected, split backend-filter from frontend-wiring.
|
||
- **Verify** should confirm PI-101..PI-121 against source; the load-bearing items are PI-111 (store filter correctness), PI-113..115 (tabs actually pass `instance.id`), and PI-117 (widget non-regression).
|
||
- This change is **independent** of the qBittorrent widgets, the storage harness, MediaIndex, the credential tester, and the Grafana gateway — none are constrained by or dependent on it.
|