fix(admin): decouple session-passkey and config-import from the HTTP request

[Yeraze]

Follow-up to #4482 / #4485 — the remaining handlers with the same blocking pattern.

Correction to what I reported on #4485

I said the affected handlers were "config export (~L650) and config import (~L1245)". The first was wrong: L650 is POST /ensure-session-passkey, not export-config. export-config reads channels from the DB/cache and never touches the passkey, so it was never affected. The actual pair is /ensure-session-passkey and /import-config.

The two handlers

POST /ensure-session-passkey existed to acquire the passkey, blocking up to 45s doing it. Now:

• If a passkey is already cached it answers 200 immediately — no mesh round-trip, which is the common case since the frontend calls this to pre-warm before config operations.
• Otherwise it returns 202 + operation id and acquires in the background.

POST /import-config on a remote node blocked on the passkey and a subsequent burst of admin sends (one per channel, plus LoRa config) — the longest hold of the three endpoints. Its body moves into a runImport thunk: local imports still await it inline and return 200, remote imports return 202 and run in the background.

The thunk's contents are deliberately left at their original indentation so the diff shows only the wrapper lines rather than a 165-line reflow. Worth knowing when reading the diff.

No new client machinery

Both reuse the operation registry and GET /operations/:id from #4485. The polling loop in ApiService is extracted into a shared followAdminOperation, and three callers now route through it:

• sendAdminCommand (unchanged behavior, just refactored onto the shared follower)
• new ensureSessionPasskey — replaces the three raw apiService.post('/api/admin/ensure-session-passkey', …) call sites in AdminCommandsTab
• importConfig's remote branch

AdminOperationResult gains an index signature so endpoint-specific payloads (import counts, passkey expiry fields) ride along in the result unchanged — the client spreads the whole result, so callers see the same shape they did before.

Testing

• Full suite: success: true, 12,946 passed / 0 failed, 0 failed suites. tsc clean, lint:ci clean with no baseline growth.
• New coverage in adminRoutes.asyncOperations.test.ts (+4): cached passkey answers 200 without opening an operation and without calling the acquisition path at all; a cold passkey returns 202 while acquisition is outstanding (mock gated on an unresolved promise) then settles with the expiry status; timeout records PASSKEY_TIMEOUT; local node still synchronous.
• Six pre-existing #4315 import-config tests needed updating — they asserted 200 and then checked a createSetLoRaConfigMessage spy. Remote import now returns 202 and the send happens after the response, so they assert 202 and await vi.waitFor(...) the spy. Their actual subject (txEnabled preservation precedence) is unchanged.

Still not hardware-validated

Same caveat as #4485: no system-test label, so the async path has not run against a real remote node. /import-config is the one I would most want exercised on hardware — it is the longest sequence and the one the Configuration Import system-test leg covers.

🤖 Generated with Claude Code

https://claude.ai/code/session_015cgD5kSurLjdeVkZ6PcQD9

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(admin): decouple session-passkey and config-import from the HTTP request

Tasks

• Read changed files and gather context
• Review adminRoutes.ts (core logic changes)
• Review api.ts (shared followAdminOperation)
• Review AdminCommandsTab.tsx (client-side changes)
• Review test files
• Post comprehensive review

---

Overall Assessment

This is a well-executed follow-up to #4485 that correctly applies the async-operation pattern to the two remaining blocking endpoints. The code is clean, the rationale is clearly documented, and the test suite properly covers the new behavior. A few items worth considering:

---

adminRoutes.ts

runImport thunk indentation (adminRoutes.ts:1226): The deliberate choice to leave the thunk body at its original indentation is pragmatic for the diff, but it creates a visually misleading code block — the body of runImport appears at the same level as the outer handler, making return { ... } at line 1390 look like a top-level return rather than the thunk's return value. A comment or doc block on runImport was added, which helps, but reviewers unfamiliar with the PR description may be confused. Consider adding a note that the indentation is intentional once this lands, or normalizing it in a follow-up.

runImport variable capture (adminRoutes.ts:1213–1216): importedChannels, loraImported, and requiresReboot are declared in the outer handler scope and mutated inside the thunk. This is technically safe since the local path awaits inline and the remote path runs in a detached closure (no re-entry), but it's a subtle coupling. If the thunk were ever made reusable or testable in isolation, the shared-state pattern would be a bug vector. Not blocking, but worth a comment.

ensure-session-passkey — no message field on 200 cache-hit response (adminRoutes.ts:652–657): The cached-passkey fast path spreads getSessionPasskeyStatus(destinationNodeNum) but the test at line 278 asserts hasPasskey: true and remainingSeconds: 250. If getSessionPasskeyStatus doesn't return a message field, then the AdminCommandAccepted.message type contract is silently not honored on the 200 path — followAdminOperation doesn't care (it passes straight through for non-202 responses), but callers who inspect .message would get undefined. Low risk in practice, but inconsistent with the 202 success path which explicitly sets message: 'Session passkey available'.

runImport's sessionPasskey variable goes stale (adminRoutes.ts:1294–1299): sessionPasskey is acquired once, then used across multiple channel sends and the LoRa fetch. If the passkey expires mid-import (they appear to have a TTL based on getSessionPasskeyStatus), subsequent sends will silently use a stale key. This was pre-existing behavior, but the async path makes it more likely to manifest on slow/unreliable mesh links since the operation runs completely in the background. Worth a // TODO at minimum.

---

api.ts

followAdminOperation — overall 120s timeout vs. mesh worst case (api.ts:1658): The comment says "comfortably above the ~75s worst case (45s passkey + 30s ACK)". For /import-config on a remote node with many channels, the actual time is: 45s passkey + (N channels × 1s pacing) + 1 more requestRemoteConfig + LoRa send = well over 75s for a 7-channel import. With 7 channels that's 45 + 7 + RTT + ... potentially ~60s total, still under 120s, but tighter than the comment implies. Consider whether the 120s budget is accurately sized for the worst-case import (8 channels + LoRa = 45 + 8 + 1.5 + RTTs), or whether the timeout should be configurable/extended.

ensureSessionPasskey type parameter is underused (api.ts:736): The method signature has <T extends { success: boolean }> but callers in AdminCommandsTab.tsx use it as ensureSessionPasskey<{ success: boolean; hasPasskey: boolean; remainingSeconds?: number }>. Since followAdminOperation already merges result into a { success: true, ...result } object, the type bound is correct — just noting it's worth keeping consistent as new fields are added to the passkey status.

AdminOperationEnvelope.data type (api.ts:166–168): AdminOperationEnvelope extends AdminOperationSnapshot and also has data?: AdminOperationSnapshot. The server wraps in { success, data } for the poll endpoint. The client dereferences via snapshot?.data ?? snapshot at line 1693 — this handles both the wrapped and unwrapped shape, which is resilient, but the type is a bit confusing (it extends the thing it also optionally contains). Not a bug, just a documentation opportunity.

---

AdminCommandsTab.tsx

No visible changes needing review — The three apiService.post('/api/admin/ensure-session-passkey', ...) call sites were correctly replaced with apiService.ensureSessionPasskey(...), which transparently follows async operations. The change is mechanical and correct.

---

Test Coverage

New async tests (adminRoutes.asyncOperations.test.ts:264–332) are comprehensive: cached fast-path (no operation), cold acquisition (202 + settle), timeout (PASSKEY_TIMEOUT), and local-node sync path. Good coverage of the new /ensure-session-passkey branch.

Pre-existing import-config remote tests updated correctly — the six #4315 tests now assert 202 and use vi.waitFor to observe the spy. Worth noting: these tests set up a manager with a pre-cached passkey (getSessionPasskey: vi.fn().mockReturnValue(new Uint8Array([1]))) so the runImport thunk path is exercised but the passkey-acquisition leg isn't. The separate ensure-session-passkey tests cover that leg directly, so overall coverage is adequate.

Missing test: import-config with a cold passkey — There's no test that exercises the full remote import path: cold passkey → acquisition → channel sends → result. The existing tests either start with a cached passkey or test passkey logic in isolation. Not critical since the component paths are individually tested, but a single integration-style test here would give more confidence before hardware validation.

---

Security

The operation ownership check at adminRoutes.ts:1934 correctly handles the null === null edge case (two session-less requesters do not share visibility). The 200/202 branching does not create a timing oracle. No new surfaces identified.

---

Summary

Approve — the implementation is correct and the PR description is unusually thorough. The items above are observations and suggestions, not blockers. The import-config cold-passkey integration test would be a nice addition but isn't required for merging. Hardware validation on /import-config is the remaining open item noted by the author.

---

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(admin): decouple session-passkey and config-import from the HTTP request

Tasks

• Read changed files and gather context
• Review adminRoutes.ts (core logic changes)
• Review api.ts (shared followAdminOperation)
• Review AdminCommandsTab.tsx and test files
• Post comprehensive review

---

Overall Assessment

This is a well-structured follow-up to #4485 that correctly applies the async operation pattern to the two remaining blocking endpoints. The implementation is clean and the PR description is unusually thorough. A few observations worth noting:

---

adminRoutes.ts

runImport indentation (adminRoutes.ts:1233): The deliberate choice to keep the thunk body at original indentation is pragmatic for the diff, but creates a visually misleading block where return { ... } at line 1404 appears to be a handler-level return rather than the thunk's. The inline comment at 1218–1231 explaining this is good — readers unfamiliar with the PR description will benefit from it.

Shared mutable state (adminRoutes.ts:1214–1216): importedChannels, loraImported, and requiresReboot are declared in the outer handler scope and mutated inside runImport. The comment at line 1228–1232 acknowledges this correctly. For the current single-call-per-request usage this is safe, but if runImport were ever hoisted into a shared helper or called twice, this would be a latent bug. The comment makes the constraint explicit, which is the right call.

Stale session passkey (adminRoutes.ts:1303–1307): The TODO comment here (#4482) is the right signal. For long remote imports over slow/retransmitting links, the passkey acquired at the start can expire before the last channel send, meaning later packets go out signed with a stale key. Running detached makes this more reachable than the previous synchronous hold. Not blocking, but a follow-up issue for re-validation mid-import would be worthwhile.

Error handling in runImport channels loop (adminRoutes.ts:1339–1343): Channel-import failures are caught-and-logged but don't fail the overall operation — the operation succeeds even if some channels failed to import. This matches the pre-existing behavior, but in the async path the caller has even less visibility into partial failures. Consider whether imported.channels count in the result gives enough signal, or whether a failedChannels field would be useful.

202 error handler is inconsistent with the success path (adminRoutes.ts:705–711 and 1455–1461): The outer catch blocks in both handlers use res.status(500).json({ error: ... }) — bare error objects, not the fail() helper from apiResponse.ts. Consistent with the pre-existing handler style in this file, but worth noting as these paths are now exposed more prominently (synchronous errors before the 202 response is sent).

---

api.ts

followAdminOperation timeout comment vs. reality (api.ts:1659–1663): The comment says the default 120s is "above the ~75s worst case" but correctly notes that remote import with many channels exceeds it, which is why importConfig passes 300_000. The comment is accurate but reads a bit backwards — it says 120s "does not cover with any margin on a retransmitting link," which is why callers pass their own budget. Good that this is documented; a reader scanning only followAdminOperation now understands why callers override the default.

ensureSessionPasskey timeout (api.ts:742–743): Uses the default 120s budget from followAdminOperation, which is correct — passkey acquisition is at most 45s, well within that window.

AdminOperationEnvelope type (api.ts:166–168): The type extends AdminOperationSnapshot and also optionally contains data?: AdminOperationSnapshot. The snapshot?.data ?? snapshot fallback at line 1699 handles both shapes. This is resilient and the comment ("The poll endpoint wraps its payload") explains the dual shape. Low concern.

Missing AbortSignal in ensureSessionPasskey (api.ts:739–743): sendAdminCommand accepts an optional { signal } for caller-driven cancellation. ensureSessionPasskey has no such parameter. For passkey acquisition this matters less (it's short-lived), but it's an asymmetry worth noting if a caller ever wants to abort during a loading-spinner cancel.

---

AdminCommandsTab.tsx

The three apiService.post('/api/admin/ensure-session-passkey', ...) call sites are correctly replaced with apiService.ensureSessionPasskey(...). The change is mechanical and correct. Call sites at lines 723, 976, and 1263 all follow the same pattern and correctly type the response.

---

Test Coverage

New async tests (adminRoutes.asyncOperations.test.ts:273–341): Comprehensive coverage of the /ensure-session-passkey async path — cached fast path (no operation created, no mesh call), cold acquisition (202 + settle), timeout → PASSKEY_TIMEOUT, and local node sync path. These are exactly the right test cases.

Cold passkey integration test added (adminRoutes.asyncOperations.test.ts:343–379): The new test covers the full remote import chain: cold passkey → 202 accepted → passkey released → channel sent → succeeded. This was the coverage gap noted in the previous review. The test confirms sendAdminCommand was not called before the gate released, which validates the operation correctly waits for acquisition.

Updated #4315 tests (adminRoutes.test.ts:264–453): The six tests correctly updated from 200 to 202 with vi.waitFor assertions on the spy. The subject of each test (txEnabled preservation precedence) is unchanged and still verified.

Missing: operation failure path for remote import-config: There's no test covering what happens when runImport throws (e.g., passkey acquisition succeeds but a channel send fails with an unrecoverable error). The operation should settle as failed with IMPORT_CONFIG_FAILED. Low priority given the channel loop catches and logs individual failures, but a test for the outer catch in the detached closure would complete coverage.

---

Security

No new surfaces. The operation ownership check (userId matching or null === null guard) carries over from #4485. The 200/202 branching doesn't introduce a timing oracle. The passkey and admin command flows are unchanged — only the HTTP-hold-to-202 split is new.

---

Summary

Approve — correct implementation, good documentation, solid test coverage. The runImport mutable-state coupling and stale-passkey TODO are the only structural items worth tracking, and both are already called out in comments. Hardware validation on /import-config remains the open item noted by the author.

---

[Yeraze]

Thanks — addressed in b4bd24a.

Poll budget vs. import worst case — good catch, and the concern is real. I checked the pacing: the remote channel loop sleeps 1000ms per channel and then does a requestRemoteConfig round-trip, all on top of the 45s passkey leg. followAdminOperation now takes an optional timeoutMs and importConfig passes 300s; the 120s default stays for single commands, where the ~75s figure is accurate. The failure mode this avoids is nasty — the client abandoning an import that is still running server-side, which looks identical to a failure but isn't.

Missing cold-passkey import test — added, and it was worth the ask. Two cases: the full chain (202 while the passkey is outstanding, nothing sent yet → release → channel sends → settled result with the import counts), and a cold-passkey timeout failing the import outright rather than partially. You were right that every existing #4315 test starts with a cached passkey, so this leg had no end-to-end coverage.

runImport shared-state coupling — documented. The comment now spells out that it mutates enclosing-scope state, runs exactly once per request, and is not reusable or independently testable as written; anything that would call it twice has to hoist that state inside first.

Stale passkey across a long import — real, and I've left a TODO(#4482) at the acquisition site rather than fixing it here. It is pre-existing behavior, but you're right that running detached makes it more reachable. The fix is a re-check/re-acquire between sends, which is a behavior change I'd rather not smuggle into a plumbing PR.

runImport indentation — acknowledged; comment expanded to warn that the closing return reads like a handler-level return but is the thunk's.

One correction: the ensure-session-passkey cache-hit path does set message. It's { success: true, message: 'Session passkey available', ...getSessionPasskeyStatus(...) } — the literal precedes the spread, so message is always present on the 200 path, same as the 202 success result. No inconsistency there.

AdminOperationEnvelope shape and the ensureSessionPasskey type bound — agreed on both, no change needed.

[Yeraze]

Round 2 addressed in fdfb59b.

Silent partial imports — fixed, and this was the best catch of the round. The channel loop absorbs per-channel errors by design, so a caller could not distinguish "1 channel" from "1 of 2 channels" — worse now that the import runs detached with no log to correlate against. The result carries failedChannels and failedChannelDetails (additive, so no consumer breaks), with a test that sends one channel successfully, has the next rejected, and asserts 1 imported / 1 failed with the failing channel named.

Bare res.status(500).json({error}) — converted both touched handlers' outer catch to fail() with machine codes (PASSKEY_FAILED, IMPORT_CONFIG_FAILED). You're right that it was pre-existing file style, but CLAUDE.md asks modified handlers to use the envelope helpers, and fail() is always safe since ApiService reads only error/code.

Missing outer-catch test — worth flagging how this resolved. I first wrote a test that faked a throw from requestRemoteConfig, and it failed: the LoRa branch catches its own errors too, so nothing escaped. The outer catch is reachable, just via the passkey throw — which the existing cold-passkey test already triggers. So rather than keep a test staged against an unreachable path, I asserted IMPORT_CONFIG_FAILED on that test. Net effect is the coverage you asked for; the test I nearly added would have been theatre.

ensureSessionPasskey lacks an AbortSignal — leaving as-is. The asymmetry is real, but no caller passes one today, and acquisition is bounded at 45s. I'd rather add the parameter when something actually wants to cancel than ship unused API surface. Easy to add later — followAdminOperation already honors signal.

runImport indentation / shared state / stale passkey / AdminOperationEnvelope — all previously addressed or acknowledged; no further change.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(admin): decouple session-passkey and config-import from the HTTP request

Tasks

• Read changed files and gather context
• Review adminRoutes.ts (core logic changes)
• Review api.ts (shared followAdminOperation)
• Review AdminCommandsTab.tsx and test files
• Review adminOperationService.ts
• Post comprehensive review

---

Overall Assessment

This is a well-polished Round 3 that has fully addressed the concerns from the two prior review passes. The implementation is correct, the documentation comments are thorough, and the test coverage is solid. A few remaining observations below, none of which are blockers.

---

adminRoutes.ts

failedChannels not populated in the local import branch (adminRoutes.ts:1267–1269): The local-node channel loop catches errors and logs them but does not push to failedChannels. The remote branch correctly calls failedChannels.push(...) inside its catch. Since failedChannels is initialized in the outer scope and returned from runImport(), a local import with a channel failure reports failedChannels: 0 while a remote import with the same failure reports failedChannels: 1. This asymmetry is pre-existing for local imports (which never reported partial failures), but the new failedChannelDetails field in the result creates an implicit contract that both branches track failures consistently. Low impact in practice — local imports are fast, failures are rare, and the log captures them — but worth a follow-up to align with the remote branch's behavior.

runImport thunk status (adminRoutes.ts:1437–1439): The remote path sets status = 'awaiting_passkey' at the start of the detached closure, but the runImport thunk itself never advances the status to 'sending' once the passkey is in hand. A client polling during a long channel-burst sees awaiting_passkey the entire time rather than sending. Minor UX issue, not a correctness problem.

void (async () => {...})() pattern (adminRoutes.ts:1437): The detached fire-and-forget IIFE is consistent with the pattern introduced in #4485 and is correct here. The unhandled-rejection risk is addressed because the inner async has a catch block. Nothing to change, just confirming the pattern is sound.

---

api.ts

ensureSessionPasskey passes straight through to followAdminOperation without logging (api.ts:739–743): This is deliberately minimal and correct — followAdminOperation handles the operation lifecycle. No issue.

followAdminOperation timeout ordering (api.ts:1675): The timeout check comes after the sleep, not before. For the default 120s budget this is fine. For very short test-driven timeouts it means the deadline can be overshot by up to delayMs (max 2000ms). Not a concern in production, just noting the behavior if someone ever passes a very small timeoutMs.

snapshot?.data ?? snapshot fallback (api.ts:1699): Handles both the { success, data } wrapped shape and the bare AdminOperationSnapshot shape correctly. Resilient and documented.

---

adminOperationService.ts

The +6/-0 changes are not directly visible in the diff summary, but from reading the file the index signature addition to AdminOperationResult is correct and well-commented ([key: string]: unknown with explanation that endpoint-specific payloads ride along in the result). The size() and clear() test seams are appropriate.

Sweep behavior on get() (adminOperationService.ts:131–133): sweep() is called on every get(), including rapid poll ticks. For a small operation map this is fine. At MAX_OPERATIONS = 500 with a 2-second poll interval, the constant iteration over all entries is negligible. No concern.

---

Test Coverage

failedChannels test (adminRoutes.asyncOperations.test.ts:381–410): Correctly tests that partial failures are counted separately from successes. The assertion on failedChannelDetails naming the specific channel is a good detail.

Cold-passkey timeout test (adminRoutes.asyncOperations.test.ts:412–437): Confirms the passkey throw propagates through the outer catch in the detached closure and records IMPORT_CONFIG_FAILED, which also implicitly verifies the outer catch doesn't lose the error to the per-channel/per-LoRa inner catches. Good.

Local node import path (adminRoutes.test.ts): The updated #4315 tests all exercise the remote path with a pre-cached passkey. There is no test for the local-node path that verifies failedChannels/failedChannelDetails are both zero on clean success, but this is very low priority since local import was not changed.

---

Security

No new surfaces. The operation ownership check (adminRoutes.asyncOperations.test.ts:448–458) covering the null === null edge case carried forward from #4485 is still present and the test at line 460 confirms it.

---

Summary

Approve — all three review rounds' concerns have been addressed. The failedChannels local-vs-remote asymmetry and the missing sending status transition are the only remaining items, both minor and both pre-existing or trivially follow-up-able. Hardware validation on /import-config remains the one open external item.

[Yeraze]

Thanks — stopping the fix loop here, with the two remaining items called out rather than silently dropped.

Missing sending status transition — real, and mine. The remote import sets awaiting_passkey and then leaves it there for the whole run, so a poller sees awaiting_passkey even while channels are actively being sent. It is cosmetic (only the mid-flight status is inaccurate; the terminal result and its payload are correct), and fixing it properly means threading an onStatus callback into runImport the way executeAdminCommand does. That is a real change to a shared thunk, and after three rounds of fixes on a plumbing PR I would rather not land it unreviewed at the end. Happy to do it as a follow-up or in this PR if preferred.

No local-path test for failedChannels: 0 — agreed it is low priority. runImport is shared, so both paths return the same shape and the remote tests cover the counting logic; the local path differs only in that it awaits inline.

followAdminOperation timeout checked after the sleep — correct reading, and deliberate: the deadline can overshoot by up to one delayMs (≤2s). Irrelevant against a 120s/300s budget, and it keeps the loop to a single sleep site. Worth knowing only if someone passes a very small timeoutMs in a test.

Everything else from all three rounds is addressed. Remaining open item is unchanged: /import-config has not been validated against real hardware.