fix(server-identity): handle onboarding persistence mismatches#1974
fix(server-identity): handle onboarding persistence mismatches#1974
Conversation
- Purpose: align the API server identity mutation with the webgui Identification flow and remove the dead onboarding-only identity path. - Before: the API sent a partial emcmd payload, kept an unused onboarding stub around, and collapsed the real emcmd failure into a generic GraphQL error. - Problem: identity updates behaved differently from the webgui path, onboarding still carried a stale implementation, and debugging transport failures was much harder than it needed to be. - Now: ServerService sends the full Identification-style payload, onboarding no longer carries the unused applyServerIdentity path, and GraphQL preserves the underlying emcmd error message in extensions.cause. - How: added webgui-style server context fields to updateServerIdentity, expanded regression coverage around payload shape and persistence side effects, and removed the orphaned onboarding helper plus its stale specs.
- Purpose: make emcmd use the same curl-over-unix-socket style transport that the webgui uses for internal /update calls.
- Before: emcmd used got against the emhttp unix socket, which could apply a command successfully and still fail afterward with low-level parse errors like 'Expected HTTP/, RTSP/ or ICE/'.
- Problem: callers such as updateServerIdentity could report a GraphQL error even after emhttp had already applied the setting change.
- Now: emcmd shells out to curl over the unix socket, preserves the existing stdout-is-error contract, and returns a response object with body/stderr metadata for callers.
- How: replaced the got socket POST with execa('curl', ... --unix-socket ... http://localhost/update), added explicit curl exit-code handling, and added direct unit coverage for successful calls, emhttp body errors, socket transport failures, and CSRF token fallback.
- Purpose: revert the curl-based emhttp transport experiment and return emcmd to the original got unix-socket client.\n- Before: emcmd posted to /update through curl and needed transport-specific flags like --http0.9 to get past emhttpd response parsing.\n- Problem: the curl path changed the transport surface area without actually solving the real bug, which is that emhttp can succeed while returning a non-empty response body that our success detection misclassifies.\n- Now: emcmd uses got.post(..., { enableUnixSockets: true }) again, matching the original helper shape while preserving the focused tests around csrf loading and response handling.\n- How: swap execa/curl back to got, restore the response object contract, and update the emcmd spec to assert the got unix-socket request path.
- Purpose: make updateServerIdentity decide success from the persisted ident.cfg state instead of trusting the emcmd response alone.\n- Before: the mutation failed immediately on emcmd transport or response errors, even in cases where emhttp had already updated ident.cfg and the change would survive reboot.\n- Problem: users saw a GraphQL error for an update that actually persisted, while the stale-file case and the persisted-success case were both collapsed into the same generic failure path.\n- Now: the resolver always checks ident.cfg after the emcmd attempt, returns success when the requested identity is present on disk, and only throws when persistence verification fails or the file still contains the old values.\n- How: read getters.paths().identConfig with ini parsing, compare NAME/COMMENT/SYS_MODEL against the requested identity, preserve the original emcmd error as context when persistence did not happen, and add regression tests for the persisted-success, stale-file, and missing-file cases.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
WalkthroughServer identity application logic shifts from onboarding service to server service with enhanced persistence and verification. New emcmd client tests validate HTTP POST behavior, CSRF token handling, and error cases. Server identity updates now include file-based verification against persisted config. Changes
Sequence DiagramsequenceDiagram
participant Service as ServerService
participant Store as Store (getters)
participant Emcmd as emcmd Client
participant FS as File System
participant Error as Error Handler
Service->>Store: Read current identity & emhttp state
Service->>Service: buildIdentityUpdateParams() with<br/>NAME, COMMENT, SYS_MODEL,<br/>server_https, server_name, server_addr
Service->>Emcmd: POST with extended params & CSRF token
alt emcmd succeeds
Service->>FS: readPersistedIdentity() from ident.cfg
FS-->>Service: Parsed identity (name, comment, sysModel)
Service->>Service: Compare requested vs persisted
alt Identity matches
Service-->>Service: Return updated server response
else Identity mismatch
Service->>Error: Throw GraphQLError with mismatch details
end
else emcmd fails
Service->>Service: Capture emcmdError, log warning
Service->>FS: readPersistedIdentity() for verification
FS-->>Service: Parsed identity
Service->>Service: Compare requested vs persisted
alt Identity matches despite emcmd failure
Service-->>Service: Log warning, return response
else Identity mismatch
Service->>Error: Throw GraphQLError with emcmdError + mismatch
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The changes introduce new file I/O patterns, error handling with fallback verification logic, and extended parameter construction. Multiple test scenarios cover positive and negative paths with file system interactions. The heterogeneous mix of new method implementations, error capture/verification flow, and expanded test coverage with temporary file setup and helper functions requires substantial individual reasoning per component. Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Purpose: make the server identity regression spec pass the full API type-check and package test matrix.\n- Before: the spec mocked getters.emhttp() with partial object literals cast directly to the emhttp slice type, which TypeScript rejected during pnpm type-check.\n- Problem: the branch-specific tests were green, but the full api/type-check gate still failed, so the package was not actually in a releasable state.\n- Now: the spec uses a typed helper that returns a full emhttp slice shape, and the array state cases use the real ArrayState enum values.\n- How: add a createEmhttpState() helper in the spec, reuse it across scenarios, and keep the mock data focused while satisfying the store slice contract.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1974 +/- ##
==========================================
+ Coverage 52.08% 52.18% +0.09%
==========================================
Files 1031 1031
Lines 71564 71575 +11
Branches 8090 8116 +26
==========================================
+ Hits 37275 37351 +76
+ Misses 34164 34099 -65
Partials 125 125 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
There was a problem hiding this comment.
💡 Codex Review
api/api/src/unraid-api/graph/resolvers/servers/server.service.ts
Lines 139 to 140 in b5c2d58
Source omitted COMMENT/SYS_MODEL from persisted identity
When callers omit comment or sysModel, these lines fill them from the in-memory emhttp slice and then always send them in the /update payload. If the slice is still unloaded/stale (it initializes empty and can lag disk), a name-only update can unintentionally write empty or outdated COMMENT/SYS_MODEL values and still pass verification because the method now checks against those derived values, not the prior persisted config.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| server_name: emhttpState.nginx?.lanName || 'localhost', | ||
| server_addr: emhttpState.nginx?.lanIp || '127.0.0.1', |
There was a problem hiding this comment.
Defaulting server_name and server_addr to localhost/127.0.0.1 means identity updates can submit synthetic host values whenever nginx state is missing (e.g., early startup or failed state load). Because these fields are now always included in changeNames=Apply, this can overwrite existing management host settings during an otherwise unrelated identity change.
Useful? React with 👍 / 👎.
Summary
This investigation started from the onboarding bug where server name and server description appeared to update in the UI but were not reliably surviving reboot.
This PR does four things:
ServerService.updateServerIdentity()send the same identity payload fields as the webgui Identification form/boot/config/ident.cfgand confirming the requestedNAME/COMMENT/SYS_MODELvalues did not persistWhat Changed
Identity update payload
The API identity mutation now mirrors the webgui Identification form payload more closely by always sending:
changeNames=Applyserver_httpsserver_nameserver_addrNAMECOMMENTSYS_MODELThis closes the gap between the original API implementation and the working webgui flow.
Persistence-first success check
updateServerIdentity()no longer trusts the emcmd response alone.Instead it:
ident.cfgNAME,COMMENT, andSYS_MODELvalues to the requested updateident.cfgis still stale, or when the file cannot be read to verify the resultError detail propagation
When the mutation does fail, the GraphQL error still includes the original low-level message in
extensions.cause, which made the transport debugging possible.Tests
Ran:
pnpm --filter ./api exec vitest run src/core/utils/clients/emcmd.spec.ts src/unraid-api/graph/resolvers/servers/server.service.spec.tspnpm --filter ./api exec vitest run src/unraid-api/graph/resolvers/customization/onboarding.service.spec.tsAdded / updated regression coverage for:
ident.cfgident.cfgduring verificationProblems We Found
1. The original API payload was underspecified
The original API
updateServerIdentity()call only sent a partial/updatepayload. It was missingserver_https,server_name, andserver_addr, while the webgui Identification form sends those fields every time.That made the API path materially different from the known-good webgui path.
2. The
/updateresponse is not cleanly parseable by our Node clientsOnce we preserved the original error text in GraphQL, we saw multiple response-shape failures while testing the real mutation against emhttpd:
got:Parse Error: Expected HTTP/, RTSP/ or ICE/curland no HTTP/0.9 allowance:curl: (1) Received HTTP/0.9 when not allowedcurl --http0.9: stdout contained a mixed success-ish payload like:<script>replaceOrigin("http://Jeffrey");</script>HTTP/1.1 200 OKheadersSo the command side effect can succeed while the response channel still looks malformed or hybrid to a normal Node HTTP client.
3. We verified this in a few steps
We confirmed the transport issue by:
scripts/emcmdhelperextensions.cause/boot/config/ident.cfggotandcurlover the emhttpd unix socketcurl --http0.9to confirm the socket response shape was the blocker, not the identity side effect itself4. The emhttpd / emcmd transport issue is real, but out of scope here
The larger issue is that
/updateover the emhttpd unix socket does not behave like a clean modern HTTP response for our Node client path.This PR does not try to solve that transport problem directly.
Instead it makes the mutation resilient to it by trusting the persisted source of truth,
ident.cfg, before returning an error.Next Big Change, Out Of Scope Here
The next substantial fix would be to harden the emhttp socket client itself so it can reliably normalize
/updateresponses without needing downstream mutations to special-case persistence verification.That likely means one of these:
/updatecurl_socketbehavior so the API and webgui share the same socket semantics/updateso programmatic callers get a machine-friendly response instead of the current mixed body / header behaviorThat is a broader transport-layer change, so I kept it out of this PR and limited the fix to server identity correctness and user-visible behavior.
Notes
There are unrelated local modifications in generated/plugin files in my worktree that are not part of this PR.
Summary by CodeRabbit
Tests
Refactor