Skip to content

Commit 3371fd4

Browse files
authored
fix(api): preserve partial results and accurate notification counts (#2123)
## Summary Preserve healthy GraphQL data when a service fails, and keep notification overview counts consistent with their lists through incremental watcher updates. Fixes #1837. Fixes #1859. ## Why This Exists A non-null Docker root can turn a service error into `data: null`, discarding healthy fields from the same query. VM domain fields already preserve partial results, but the VM root also needs a nullable boundary for root-level errors. The legacy notifier writes each new notification into both unread and archive. The archive list hides unread copies, while its counters count both. Arithmetic based only on add events also drifts when a move overwrites an existing archive copy or a removal reveals a hidden copy. ## Resolution Make the Docker and VM query roots nullable while retaining field errors. Update the published schema and affected client types. Keep a compact per-ID index containing the unread and archive severities. Watcher add/change/unlink events reread only that ID's two possible files and apply the difference between its previous and current visible category. Moves and deletes reconcile affected IDs before returning, independently of later watcher events. No ordinary event or mutation performs a full count reconciliation. Startup and explicit reconciliation rebuild the index from disk. Startup waits for watcher readiness and replays changes received during hydration. A failed reconciliation preserves the last valid counts and index instead of publishing a partial zeroed snapshot. ## Reviewer Considerations - Ordinary count updates read at most two notification copies per affected ID, independent of archive history. Bulk operations reconcile their affected IDs. - The existing warning/alert broadcaster still reads unread notifications. Its payload updates and notification-added events remain covered separately from count updates. - The index stores IDs and severities, not notification payloads. It resets when the configured directory changes. - Notification storage, legacy file format, authorization, and persistent-notification semantics are unchanged. - Existing nullable VM domain fields retain their partial result shape, and GraphQL errors remain visible. ## Behavior Changes - A failed Docker subtree no longer erases other successful root data. - Counts exclude hidden archive copies, remain stable on repeated events, and follow severity changes, removals, and overwrite moves. - Mutation responses contain reconciled counts before delayed watcher callbacks arrive. ## Verification - Six targeted suites: 106 tests passed, covering GraphQL partial results, Docker/VM resolvers, authorization, and notifications. - 21 notification integration cases passed with native watching and again with `CHOKIDAR_USEPOLLING=true`. They use real filesystem changes, Chokidar, PubSub subscribers, and GraphQL HTTP queries/mutations. - Coverage includes both legacy write orders; creation; archive/restore/delete and filtered/bulk actions; overwrite renames; severity/content changes; subscription snapshots; bursts; startup and reconciliation races; directory changes; failed moves; delayed unlink events; and read-failure recovery. - A history-size regression seeds 128 archive files and proves ordinary events perform no full recount or reads of those historical files. - The expanded tests fail against the previous implementation for event recounts, change handling, and failed reconciliation behavior. - API `pnpm type-check`, `pnpm lint` including authorization lint, and `pnpm build` passed. - Prior API/web code generation completed. Web type-check had the same 134 errors with candidate and baseline generated types, with no added errors. Shared UI build remains blocked by the existing unresolved `@vue/language-core` dependency. - Legacy duplicate-file behavior is exercised with real files. Public creation exercises the existing local-write fallback when the appliance notifier executable is unavailable. Live counter acceptance is documented below; no production deployment is claimed. ### Native appliance validation - **Passed on a managed, licensed Unraid 7.3.2 VM** using the production-shaped PR plugin from CI run `40211461696`, exact source commit `fdef73bba0995652a8c29cd18cda9dd3d5bec191`. - All **11 checks passed** in `api/scripts/qa/notification-counter.mjs`: baseline; actual native dual writes at all severities; native archive; API restore; API archive over an existing copy; API archive deletion; external severity change; external unread deletion revealing the archive's original severity; native archive-only delivery; a burst of 24 native file writes; and fixture cleanup. - Each check compared cached counters against complete visible GraphQL lists and independent disk state. The run never invoked `recalculateOverview`. Delivery used the native script's browser-only override. - Verified the published PLG against CI (`cfa341280d58b6258229965bafbbaa395695034d162f0e158cfaa1afd05bd72b`), the guest's cached TXZ (`b28a340ebefd98152ac634e9c78e8e058638694393470c44a572b7536d757ec7`), and **all 32 installed API dist files** against that payload. - Actual appliance notifier SHA-256: `665fe6d4970f610172e4eb7c9e9598f1bd84544043d0021f3b7496f0babb2377`. - Notification fixtures were removed, the temporary API key was deleted through the native CLI, and the provider completed cold rollback and release. The VM was not retained. - Current notification suite: 66 local tests pass, plus API type-check, lint, commit hooks, and guest-script syntax check. Exact-head CI completed successfully; dependency audit remains an advisory failure. - This proves live notification counter behavior through the native script and GraphQL. No browser or live subscription acceptance is claimed by this appliance run. ### Adversarial appliance validation (current candidate) - Tested `29a80c18a35deb9df2b038962f571974d6d47115` with the production-shaped plugin from CI run `40215498199` on a managed, licensed Unraid **7.4.0-rc.1** VM. CI and CodeQL passed; dependency audit is an advisory failure. - **35 recorded checkpoints completed.** Counters matched independent disk state and complete GraphQL lists throughout. Passed actual native API creation and metadata persistence; all three authenticated WebSocket subscriptions; unchanged-severity content updates; add-event delivery without duplicate native events; repeated native tickets; selected-ID, severity-filtered and all-item bulk archive/restore; deletion; persisted native files after service restart; and subscription reconnection. - Replaced the unread directory with a regular file before restarting the API. The public request exposed the initialization failure; after restoring the directory, public reads and mutations recovered without another process restart. - Lowered the guest's inotify watch limit, required a real logged `ENOSPC` from a new watched subdirectory, restored the limit, and proved that the same API process still counted real native writes. This validates the lifetime watcher error listener. - Verified the PLG (`c0d47c6007aa8e2b4ec2dd021c8b0a293fe1bdc3a4923d4676e388be5d52e99b`), cached TXZ (`80a526aa073bd4c3c770460f5c73628c68c603e538499a01b76b759a1d75b326`), and all 32 installed API dist files against the CI payload. Native notifier hash remained `665fe6d4970f610172e4eb7c9e9598f1bd84544043d0021f3b7496f0babb2377`. - Local validation: 121 targeted tests pass; all 24 filesystem integration cases also pass with polling enabled. API type-check, lint, build and commit hooks pass. Regression tests reproduced watcher initialization/runtime failures before the fixes. - Cleanup verified no temporary key and no remaining isolated notification files. Original settings were restored byte-for-byte. Provider cold rollback/release completed; the guest is not retained. ### Open pre-existing creation finding **The adversarial run is `passed_with_findings`, not a clean pass, and intentionally exited nonzero.** When the installed native notifier succeeds, `createNotification` returns a generated UUID-based ID that differs from the actual native disk record's ID. Creation side effects, metadata, subscriptions and subsequent operations using the listed ID work; clients using the creation response's ID do not have the real record ID. The pre-PR source at `e160a4ea136e56f7bab68ce9e3b6b8cc73b3aac7` already generates this ID, passes the title to the native script, and returns the generated ID. Those operations are unchanged by this PR. This classification uses source comparison, not a baseline appliance run. The finding remains unresolved and is separate from the counter and watcher lifecycle fixes. ### Final QA script diagnostics The QA-script diagnostics commit `d540c61734a6abad4cb0a72663050a74c6929e48` differs from the tested `29a80c18a` only in the guest test script. API source trees are identical (`14806261103820a95bd96ddffbf75b98649bb855`). The script now preserves the original startup failure if restoration also fails, records restoration failure separately, and uses byte offsets when scanning logs. Targeted local checks of the actual script passed for ASCII and multibyte log prefixes and both startup/cleanup failure combinations. These diagnostic corrections were not rerun on an appliance; the full appliance evidence remains pinned to `29a80c18a` above. ### Burst read-cost investigation before scheduling fix Added 12 real-filesystem/Chokidar/PubSub cost-characterization tests. They exercise 24-event bursts with empty and 1,000-unread/1,000-archive history, plus 100-event bursts with that larger history. Operations cover native-style dual-file creation, external content/severity changes, unread deletion revealing archive copies, and archive-only creation. All 12 pass with native watching and again with polling; the complete targeted set now has 133 passing tests across 8 files. API type-check and lint pass. Measurement covers service notification-content read attempts after hydration, including warning/alert payload scans. Independent list verification occurs afterward. For 100 content/severity changes with 1,000 existing unread history files, both modes recorded **110,200 read attempts and 100 warning/alert scans**. Peak simultaneous scans were 47 with native watching and 48 with polling; each local event workload took about four seconds. An archive-only burst of 100 files required 200 read attempts and zero warning scans. Counters, final payloads, the 50-item warning limit, and notification-added uniqueness remained correct. Unrelated archive history was not reread. The tests enforce a per-event-plus-payload-scan ceiling, but they **do not** establish a constant per-burst budget or acceptable appliance latency. OS disk I/O, watcher metadata operations, and production hardware latency were not measured. No new QA VM acceptance is claimed. These commits change only tests and documentation; prior packaged API evidence remains applicable to the unchanged application source. **Open performance concern:** unread events can launch overlapping full unread payload scans. This investigation documented amplification before the scheduling fix described below. ### Warning/alert refresh scheduling fix Warning/alert payload publication now shares one running refresh. Requests received before the scan combine after queued counter work; requests arriving during reading or publication trigger a subsequent serial refresh. There is no new timer, payload cache, schema, or file-format change. Per-ID count reconciliation and per-file notification-added delivery remain intact. Six deterministic scheduling regressions cover simultaneous calls, a real mid-read disk change, requests during publication, requests during a failed read, and next-request recovery after read/publication failure. Before this fix, three overlap regressions failed with 101 concurrent/duplicate scans. The burst tests now require at most one active scan, at most four scans for each large-history test burst, correct final counts/payloads, and no unrelated archive payload reads. For 100 content/severity changes with 1,000 existing unread files, native watching measured **1,300 content-read attempts and one scan**, versus 110,200 and 100 scans before. Polling measured one or two sequential scans (2,400 attempts observed). These are local file-read attempts, not physical disk I/O or appliance latency measurements. Sustained events can cause further sequential scans; four scans is a test-fixture budget, not an unbounded-stream guarantee. Validation: **139 targeted tests pass with polling**, including all 42 notification scheduling/filesystem/load cases. Isolated native watcher integration and native load/scheduling tests pass. API type-check, lint/authorization lint, and production build pass. Combined local native runs intermittently miss raw Chokidar add events; the previous revision reproduces the same failure under the combined workload (131/133 pass). That local test limitation remains disclosed; it was not fixed by changing refresh scheduling. The prior open amplification finding is addressed by this implementation and the stricter budgets. Current application source has changed: **fresh CI and packaged managed-VM validation are recorded below**, and the earlier appliance receipts continue to prove their pinned revisions only. The pre-existing native creation ID finding remains separate. No merge or production deployment. ### Exact-head appliance validation after refresh scheduling Validated `acd1994b4ec8e48ac6f1a19dd006c5f7932cff0e` using the production-shaped plugin from CI run `40232164901` on a managed, licensed **Unraid 7.4.0-rc.1** VM. Main CI, including Test API, and CodeQL run `40232164782` passed. Dependency audit remains an advisory failure; release and production deployment jobs were intentionally skipped. The same **35 adversarial checkpoints completed**: native creation and dual-file delivery, counts/list/disk agreement, all three authenticated live GraphQL subscriptions, content updates at unchanged severity, no duplicate additions, bulk and filtered archive/restore/delete, service-restart persistence and reconnection, real startup-directory failure recovery without another restart, and real logged inotify `ENOSPC` while the same API process remained operational. Verified the CI PLG (`ec40e39f25354325bf8ba781f6c04f809fff5ca43e50c5bc32d00c2a9c777477`), cached TXZ (`3d5dfb3229fcd0967692c77092b4ff16470bfef3e80d6a08e22d5b8f2d07986a`), and **all 32 installed API dist files** against that package. The installed entrypoint hash was `08844a7a9e65e3f889b67123ac6dee1631e5481c1b1efeb5cf1f716f49ac3853`. **Result: `passed_with_findings`, intentionally exit 1.** The existing native creation response ID still differs from the stored record ID. This was already present in pre-PR source; it remains a separate unresolved finding. All other recorded checkpoints passed. This run validates appliance behavior after scheduling changed; the 100-event file-read reduction remains local test evidence, not appliance disk-I/O profiling. Cleanup verified that the temporary key was absent, isolated notification files numbered zero, the inotify limit was restored, and the original notification configuration was restored byte-for-byte. Provider cold rollback/release completed and advanced its generation. A fresh reservation listing confirmed this task's reservation was absent. The shared capacity was subsequently acquired by another job; it was left under that job's ownership. The CI waiter was verified, unloaded and cleaned up. No merge or production deployment. ## Risk Nullable roots affect generated client contracts. The notification index uses memory proportional to stored IDs; ordinary count updates avoid history scans. Read failures retain the last valid state and are reported rather than silently replacing counts with zeros. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * VM and Docker queries can now return partial results when either service is unavailable, rather than requiring both query roots to be present. * Notification counts and updates stay synchronized with notification files during changes and bulk operations. Notification reads and updates can recover after watcher initialization failures. * **Documentation** * Added guidance for running notification-counter checks on a managed appliance, including optional extended checks and how to interpret findings. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
1 parent b95ae92 commit 3371fd4

14 files changed

Lines changed: 2002 additions & 252 deletions

‎api/generated-schema.graphql‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3314,7 +3314,7 @@ type Query {
33143314
vars: Vars!
33153315

33163316
"""Get information about all VMs on the system"""
3317-
vms: Vms!
3317+
vms: Vms
33183318
parityHistory: [ParityCheck!]!
33193319
array: UnraidArray!
33203320
customization: Customization
@@ -3326,7 +3326,7 @@ type Query {
33263326

33273327
"""Network interfaces"""
33283328
networkInterfaces: [InfoNetworkInterface!]!
3329-
docker: Docker!
3329+
docker: Docker
33303330
disks: [Disk!]!
33313331
assignableDisks: [Disk!]!
33323332
disk(id: PrefixedID!): Disk!

‎api/scripts/qa/README.md‎

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,123 @@
1+
# Notification counter appliance test
2+
3+
## Local burst and read-cost tests
4+
5+
From the repository root, run:
6+
7+
```sh
8+
pnpm --filter ./api test src/unraid-api/graph/resolvers/notifications/notifications.load.spec.ts
9+
CHOKIDAR_USEPOLLING=true pnpm --filter ./api test src/unraid-api/graph/resolvers/notifications/notifications.load.spec.ts
10+
```
11+
12+
These tests use real temporary files, Chokidar events, notification readers,
13+
and PubSub subscriptions. They cover creation of both native file copies,
14+
content/severity changes, unread deletion revealing archive copies, and
15+
archive-only creation. Fixtures include 1,000 unread plus 1,000 archive history
16+
files and bursts of 24 or 100 notifications.
17+
18+
Each `notification-load` record reports file-content read attempts, warning/alert
19+
scan count, peak simultaneous scans, and elapsed time. Measurement starts after
20+
hydration and ends after handlers and payload publications finish. Ground-truth
21+
list queries run afterward and do not inflate the recorded event cost. Read
22+
attempts include missing-copy reads; they are not physical disk I/O measurements.
23+
Watcher metadata reads and other processes are outside this measurement.
24+
25+
Assertions check final counters against full lists, final warning payloads,
26+
deduplicated additions, no history recount, no reads of unrelated archived
27+
payloads, and no more than two count reads per event plus the unread payload
28+
scans. They require at most one active payload scan and at most four scans for
29+
each large-history test burst. Archive-only bursts must trigger no warning/alert
30+
scans. This limits the fixture's total read budget rather than only its counter
31+
reads. Separate broadcast tests hold reads/publication open to prove that
32+
overlapping requests combine, a mid-scan request produces a fresh final payload,
33+
and read/publication failures do not block later requests.
34+
35+
Before refresh scheduling, 100 changes with 1,000 unread history files caused
36+
110,200 content-read attempts and 100 overlapping scans. With scheduling, the
37+
same native-watcher workload used 1,300 attempts and one scan in a local run;
38+
polling used 2,400 attempts and two sequential scans. Incoming events during a
39+
scan can require a follow-up scan; there is no fixed scan-count guarantee for an
40+
unbounded stream of events. Elapsed times remain diagnostic, not machine-dependent
41+
CI limits. These local tests do not establish appliance latency or physical disk
42+
I/O performance. Appliance acceptance remains a separate step.
43+
44+
## Appliance setup
45+
46+
`notification-counter.mjs` tests the installed API against Unraid's native
47+
`/usr/local/emhttp/webGui/scripts/notify`. Run it inside a disposable, managed
48+
QA guest after installing the production-shaped plugin for the selected commit.
49+
The guest must have `/boot/config/qa-harness-ok` and Node.js with native `fetch`.
50+
51+
The QA runner must establish the source commit and package digest before
52+
installation. Extract the API entrypoint from that same package and calculate
53+
its SHA-256. Supply that expected hash to the guest; do not calculate the
54+
expected value from the guest's installed file. The script compares the
55+
installed entrypoint with this hash and records the native notifier's hash.
56+
This comparison covers the entrypoint. The runner remains responsible for
57+
verifying the complete installed package and its revision.
58+
59+
Supply these environment variables through the managed guest execution path:
60+
61+
- `QA_GRAPHQL_URL`: authenticated local GraphQL endpoint for this guest.
62+
- `QA_API_KEY`: temporary key with notification read, update, and delete permissions.
63+
- `QA_API_ENTRYPOINT`: absolute installed API entrypoint path from the package.
64+
- `QA_API_SHA256`: expected SHA-256 from the pinned package.
65+
- `QA_SOURCE_REVISION`: full 40-character source commit for that package.
66+
- `QA_REPORT_PATH`: fresh absolute path for the private JSON receipt.
67+
68+
Run `node /absolute/guest/path/notification-counter.mjs`. Use the QA VM
69+
provider's current lifecycle and execution contract. Do not run this script on
70+
a production server. The runner must restore and release the guest even when
71+
the test fails, and remove its temporary API key.
72+
73+
The script uses `notify -i "<severity> 1"` to enable browser-file delivery only.
74+
It does not change notification settings or invoke email and notification agents.
75+
It creates uniquely named files and removes only those files in `finally`.
76+
It verifies:
77+
78+
- All three severities through the actual notifier, including both unread and
79+
archive copies of each notification.
80+
- Native archive actions, API restore, API archive over an existing copy, and API deletion.
81+
- External severity changes and unread deletion that reveals the archive's severity.
82+
- Native archive-only delivery and a burst of 24 native file writes.
83+
- Matching cached counters, visible GraphQL lists, and independent disk counts
84+
after every step and after cleanup.
85+
86+
The script never calls `recalculateOverview`, so a watcher failure cannot be
87+
hidden by a manual refresh. Each check allows 15 seconds for watcher delivery.
88+
It stops if notification ground truth is ambiguous or a list reaches its
89+
10,000-item cap. Existing notifications must have a supported importance value.
90+
The receipt contains hashes, source revision, fixture counts, and check names;
91+
it excludes credentials, the endpoint, and existing notification contents.
92+
93+
A syntax check is not appliance acceptance. Only a receipt with `status=passed`
94+
and `cleanup=complete`, plus the runner's package and VM lifecycle evidence,
95+
proves that this appliance test passed.
96+
97+
## Adversarial appliance pass
98+
99+
Set `QA_ADVERSARIAL=true` and `QA_ALLOW_SERVICE_RESTART=true` to run the extended
100+
pass. This requires a disposable guest, an isolated empty notification directory,
101+
and native notification settings `normal=1`, `warning=1`, and `alert=1`. The runner
102+
must prepare those settings before restarting the API, preserve the original
103+
configuration, and restore it afterward. The temporary API key also needs
104+
notification creation permission. With the current CLI, use an alphanumeric key
105+
name, an explicit `GUEST` role, and the four explicit notification permissions.
106+
107+
The extended pass checks API creation through the installed native script,
108+
three authenticated GraphQL WebSocket subscriptions, content updates without
109+
count changes, selected-ID and severity-filtered bulk archive/restore, all-item
110+
archive/restore/deletion, persisted counts after service restart, subscription
111+
reconnection, and recovery from a real startup directory failure without
112+
another process restart.
113+
114+
It also temporarily lowers the guest's inotify watch limit, creates a watched
115+
subdirectory, requires a logged `ENOSPC`, restores the original limit in
116+
`finally`, and proves that the same API process still counts native writes.
117+
The runner must restore and release the VM on every outcome. This fault test
118+
must never run on a shared or production guest.
119+
120+
The receipt separately records whether the ID returned by native API creation
121+
matches the actual disk record. A mismatch produces `passed_with_findings`,
122+
which is not a clean regression verdict. The runner must report that finding
123+
and establish whether it predates the candidate before deciding its scope.

0 commit comments

Comments
 (0)