Merged
Conversation
Contributor
WalkthroughAdds cache-backed internal boot-device state with TTL and in-flight deduplication, a public cache invalidation method, and tests covering caching, concurrency, and invalidation. Onboarding invokes cache invalidation after boot configuration. UI/store and tests adjust TPM GUID/transfer visibility logic and test setup consolidation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as InternalBootStateService
participant Cache as Cache Manager
participant Disks as DisksService
Client->>Service: getHasInternalBootDevices()
Service->>Cache: get("internal-boot-device-exists")
Cache-->>Service: miss
par concurrent callers
Client->>Service: getHasInternalBootDevices()
Service->>Service: sees in-flight promise -> await
and single loader
Service->>Disks: getDisks()/getArrayData()
Disks-->>Service: disk list / boot info
Service->>Service: compute boolean result
Service->>Cache: set(key, result, TTL)
Cache-->>Service: stored
Service->>Service: resolve in-flight promise
end
Service-->>Client: result (cached / resolved)
Client->>Service: invalidateCachedInternalBootDeviceState()
Service->>Cache: del(key)
Cache-->>Service: deleted
Service->>Service: clear in-flight marker
Service-->>Client: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 unit tests (beta)
📝 Coding Plan
Comment |
elibosley
force-pushed
the
codex/share-internal-boot-state
branch
from
ba5a6bd to
ba27610
Compare
💡 Codex Review Determine flash boot state from mount, not boot.type
This check can never report ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/src/unraid-api/graph/resolvers/disks/internal-boot-state.service.ts`:
- Around line 39-41: The invalidateCachedInternalBootDeviceState() currently
clears the cache key and nulls pendingHasInternalBootDevicesLookup but doesn't
prevent an in-flight loadHasInternalBootDevices() from later repopulating the
cache with stale data; fix by adding a generation token (e.g.,
this.internalBootLookupGeneration: number) that
invalidateCachedInternalBootDeviceState() increments, then have
loadHasInternalBootDevices() capture the current generation into a local
variable and only write to cache and clear pendingHasInternalBootDevicesLookup
if the captured generation still equals this.internalBootLookupGeneration;
update references to pendingHasInternalBootDevicesLookup and
INTERNAL_BOOT_DEVICE_SETUP_CACHE_KEY accordingly and add a regression test that
calls invalidateCachedInternalBootDeviceState() before the first lookup resolves
to ensure the stale result does not repopulate the cache.
In
`@api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.ts`:
- Line 410: The call to
this.internalBootStateService.invalidateCachedInternalBootDeviceState() must be
treated as best-effort so its failure cannot flip a successfully completed
mkbootpool flow into an overall failure; wrap that await in its own try/catch,
catch any error and log it as a warning (do not rethrow or return a failure),
then continue returning the successful result from the surrounding handler
(i.e., keep cmdCreatePool, cmdAssignDisk and cmdMakeBootable outcomes as
authoritative). Locate the call to invalidateCachedInternalBootDeviceState in
the onboarding-internal-boot service method and replace the direct await with a
try { await
this.internalBootStateService.invalidateCachedInternalBootDeviceState(); } catch
(err) { /* log warning via existing logger/processLogger */ } so cache
invalidation errors are surfaced as warnings only.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b90fac45-751c-4971-8719-839cf0e0f83d
📒 Files selected for processing (6)
api/src/unraid-api/graph/resolvers/disks/internal-boot-state.service.spec.tsapi/src/unraid-api/graph/resolvers/disks/internal-boot-state.service.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.spec.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.tsweb/__test__/components/Registration.test.tsweb/src/components/Registration.standalone.vue
api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.ts
Outdated
Show resolved
Hide resolved
Contributor
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1921 +/- ##
==========================================
+ Coverage 51.50% 51.55% +0.04%
==========================================
Files 1024 1024
Lines 70790 70846 +56
Branches 7803 7814 +11
==========================================
+ Hits 36459 36522 +63
+ Misses 34209 34202 -7
Partials 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.spec.ts`:
- Around line 227-251: The Logger.prototype.warn spy created as warnSpy in the
test leaks into other tests because it's not restored; update the test (the spec
containing the it('returns success when cache invalidation fails after setup and
logs a warning' ...) block) to restore the spy after assertions by calling
warnSpy.mockRestore() (or ensure restoration in a finally/afterEach cleanup) so
the Logger.prototype.warn implementation is returned to its original behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b9d9b31-0128-4735-b6be-201ca3bb1f64
📒 Files selected for processing (7)
api/src/unraid-api/graph/resolvers/disks/internal-boot-state.service.spec.tsapi/src/unraid-api/graph/resolvers/disks/internal-boot-state.service.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.spec.tsapi/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.tsweb/__test__/store/server.test.tsweb/src/components/Registration.standalone.vueweb/src/store/server.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.ts
- api/src/unraid-api/graph/resolvers/disks/internal-boot-state.service.spec.ts
Comment on lines
+227
to
251
| it('returns success when cache invalidation fails after setup and logs a warning', async () => { | ||
| const warnSpy = vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); | ||
| vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited<ReturnType<typeof emcmd>>); | ||
| internalBootStateService.invalidateCachedInternalBootDeviceState.mockRejectedValue( | ||
| new Error('cache delete failed') | ||
| ); | ||
| const service = createService(); | ||
|
|
||
| const result = await service.createInternalBootPool({ | ||
| poolName: 'cache', | ||
| devices: ['disk-1'], | ||
| bootSizeMiB: 16384, | ||
| updateBios: false, | ||
| }); | ||
|
|
||
| expect(result.ok).toBe(true); | ||
| expect(result.code).toBe(0); | ||
| expect(vi.mocked(emcmd)).toHaveBeenCalledTimes(4); | ||
| expect(internalBootStateService.invalidateCachedInternalBootDeviceState).toHaveBeenCalledTimes( | ||
| 1 | ||
| ); | ||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| 'Failed to invalidate cached internal boot device state after successful setup: cache delete failed' | ||
| ); | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Vitest v3, does vi.clearAllMocks() restore vi.spyOn() implementations on class prototypes, or should vi.restoreAllMocks() / spy.mockRestore() be used?
💡 Result:
In Vitest v3, vi.clearAllMocks() does not restore vi.spyOn() implementations (including spies placed on class prototypes)—it only clears call/instance history and keeps the mocked implementation in place. [1]
To put a spied prototype method back to its original implementation/descriptor, use vi.restoreAllMocks() (restores all spies) or spy.mockRestore() (restores one spy). [1][2]
Sources: [1] [2]
🏁 Script executed:
fd -t f "onboarding-internal-boot.service.spec.ts" -x cat -n {}Repository: unraid/api
Length of output: 14868
Restore the Logger.prototype.warn spy after this test.
Line 228 creates a prototype spy but never restores it. vi.clearAllMocks() only clears call history—it does not restore spy implementations—so the mocked behavior leaks into subsequent tests.
🔧 Proposed fix
it('returns success when cache invalidation fails after setup and logs a warning', async () => {
const warnSpy = vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined);
- vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited<ReturnType<typeof emcmd>>);
- internalBootStateService.invalidateCachedInternalBootDeviceState.mockRejectedValue(
- new Error('cache delete failed')
- );
- const service = createService();
-
- const result = await service.createInternalBootPool({
- poolName: 'cache',
- devices: ['disk-1'],
- bootSizeMiB: 16384,
- updateBios: false,
- });
-
- expect(result.ok).toBe(true);
- expect(result.code).toBe(0);
- expect(vi.mocked(emcmd)).toHaveBeenCalledTimes(4);
- expect(internalBootStateService.invalidateCachedInternalBootDeviceState).toHaveBeenCalledTimes(
- 1
- );
- expect(warnSpy).toHaveBeenCalledWith(
- 'Failed to invalidate cached internal boot device state after successful setup: cache delete failed'
- );
+ try {
+ vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited<ReturnType<typeof emcmd>>);
+ internalBootStateService.invalidateCachedInternalBootDeviceState.mockRejectedValue(
+ new Error('cache delete failed')
+ );
+ const service = createService();
+
+ const result = await service.createInternalBootPool({
+ poolName: 'cache',
+ devices: ['disk-1'],
+ bootSizeMiB: 16384,
+ updateBios: false,
+ });
+
+ expect(result.ok).toBe(true);
+ expect(result.code).toBe(0);
+ expect(vi.mocked(emcmd)).toHaveBeenCalledTimes(4);
+ expect(
+ internalBootStateService.invalidateCachedInternalBootDeviceState
+ ).toHaveBeenCalledTimes(1);
+ expect(warnSpy).toHaveBeenCalledWith(
+ 'Failed to invalidate cached internal boot device state after successful setup: cache delete failed'
+ );
+ } finally {
+ warnSpy.mockRestore();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('returns success when cache invalidation fails after setup and logs a warning', async () => { | |
| const warnSpy = vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); | |
| vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited<ReturnType<typeof emcmd>>); | |
| internalBootStateService.invalidateCachedInternalBootDeviceState.mockRejectedValue( | |
| new Error('cache delete failed') | |
| ); | |
| const service = createService(); | |
| const result = await service.createInternalBootPool({ | |
| poolName: 'cache', | |
| devices: ['disk-1'], | |
| bootSizeMiB: 16384, | |
| updateBios: false, | |
| }); | |
| expect(result.ok).toBe(true); | |
| expect(result.code).toBe(0); | |
| expect(vi.mocked(emcmd)).toHaveBeenCalledTimes(4); | |
| expect(internalBootStateService.invalidateCachedInternalBootDeviceState).toHaveBeenCalledTimes( | |
| 1 | |
| ); | |
| expect(warnSpy).toHaveBeenCalledWith( | |
| 'Failed to invalidate cached internal boot device state after successful setup: cache delete failed' | |
| ); | |
| }); | |
| it('returns success when cache invalidation fails after setup and logs a warning', async () => { | |
| const warnSpy = vi.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); | |
| try { | |
| vi.mocked(emcmd).mockResolvedValue({ ok: true } as Awaited<ReturnType<typeof emcmd>>); | |
| internalBootStateService.invalidateCachedInternalBootDeviceState.mockRejectedValue( | |
| new Error('cache delete failed') | |
| ); | |
| const service = createService(); | |
| const result = await service.createInternalBootPool({ | |
| poolName: 'cache', | |
| devices: ['disk-1'], | |
| bootSizeMiB: 16384, | |
| updateBios: false, | |
| }); | |
| expect(result.ok).toBe(true); | |
| expect(result.code).toBe(0); | |
| expect(vi.mocked(emcmd)).toHaveBeenCalledTimes(4); | |
| expect( | |
| internalBootStateService.invalidateCachedInternalBootDeviceState | |
| ).toHaveBeenCalledTimes(1); | |
| expect(warnSpy).toHaveBeenCalledWith( | |
| 'Failed to invalidate cached internal boot device state after successful setup: cache delete failed' | |
| ); | |
| } finally { | |
| warnSpy.mockRestore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@api/src/unraid-api/graph/resolvers/onboarding/onboarding-internal-boot.service.spec.ts`
around lines 227 - 251, The Logger.prototype.warn spy created as warnSpy in the
test leaks into other tests because it's not restored; update the test (the spec
containing the it('returns success when cache invalidation fails after setup and
logs a warning' ...) block) to restore the spy after assertions by calling
warnSpy.mockRestore() (or ensure restoration in a finally/afterEach cleanup) so
the Logger.prototype.warn implementation is returned to its original behavior.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores