fix(cli)(#21): throw ExitSignal from process.exit wrapper so handlers stop#22
Merged
Merged
Conversation
added 2 commits
May 23, 2026 13:38
… stop
`legacy-cli.ts`'s `main()` wraps `process.exit` to destroy the Sphere
instance (Nostr relays, IPFS handles, SQLite) before the real exit.
Cleanup is async, so the wrapper used to schedule `inst.destroy()
.finally(originalExit)` and `return undefined as never`. That left the
calling line of code to continue executing past `process.exit(N)`.
The shape that surfaced this was `invoice-status`:
if (matched.length === 0) {
console.error('No invoice found matching prefix: ...');
process.exit(1); // wrapper schedules destroy, returns
}
const invoiceId = matched[0].invoiceId; // ← matched[0] undefined
…which crashed with `Cannot read properties of undefined (reading
'invoiceId')`. Every other `invoice-*` handler (close, cancel, pay,
return, receipts, notices, transfers, export) shares the same shape,
and there are ~180 `process.exit(N)` call sites across this file that
all depend on synchronous termination.
The wrapper now throws an `ExitSignal` sentinel synchronously when a
Sphere instance is loaded. The outer try/catch in `main()` detects
`ExitSignal`, awaits `closeSphere()`, and forwards the code through
the original (non-wrapped) `process.exit` so the catch is not re-
entered. When no instance is loaded (early arg-validation paths), the
wrapper falls straight through to `originalExit`, matching the
previous synchronous behaviour for help / usage exits.
ExitSignal deliberately does not extend `Error` so inner
`catch (err)` blocks that filter on `err instanceof Error` do not
classify it as a normal error worth logging. Every inner catch in
this file either re-calls `process.exit(N)` (which re-throws an
ExitSignal that propagates correctly) or sits over a try body with
no `process.exit` (so ExitSignal can never reach it) — audited via
the catch-block sweep in `src/legacy/legacy-cli.ts`.
Regression pins in `test/integration/cli-invoice.integration.test.ts`
(lifecycle block, gated by `integrationSkip`):
- `invoice status <unknown-prefix>` → exit 1 + clean error message,
no `Cannot read properties of undefined` / `TypeError` anywhere.
- Companion `it.each` for `close` / `cancel` / `pay` — same shape,
catches a wrapper regression surfacing on any of these handlers.
Manual repro (matches issue body) on testnet:
sphere wallet create alice
sphere wallet use alice
sphere init --network testnet --nametag inv-crash-xxx
sphere invoice status 00005eb450a21d54f6d77b3c352a26a7539cc453ccdb1d1928dcdb6a0a266ca31e82
→ No invoice found matching prefix: …
→ exit 1, no stack trace ✓
The prior bf40221 fix (`fix(invoice)(#226)`) added an explicit
`return` after the catch-block `process.exit(1)` in invoice-deliver.
That `return` is now unreachable (the throw propagates first) but
left in place as a defensive marker — removing it widens this PR's
scope unnecessarily.
Related: #19 / #20 (daemon detach) surfaced this bug indirectly by
unblocking manual-test-full-recovery.sh §B → §C.4, where peer2-alice
hit `invoice status` for an invoice it had never received. The
cross-device invoice sync gap (peer2 not seeing peer1's invoice) is
the SDK-side follow-up tracked separately; this commit only fixes
the CLI crash that was masking it.
… guard The defensive `return` in invoice-deliver's catch dates from #226 when the `process.exit` wrapper returned `undefined as never` and required explicit returns at every call site. With #21's wrapper rewrite the ExitSignal throw propagates first, so the comment's "wrapper returns undefined" wording is now wrong and would confuse a future reviewer auditing why the `return` is there. Keep the `return` itself — defensive marker against a wrapper regression that reintroduces the fall-through. Rewrite the comment to describe the current ExitSignal-based mechanism and the regression class it guards against. No behaviour change. Pure documentation drift cleanup.
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.
Closes #21.
Summary
sphere invoice status <unknown-prefix>(and every otherinvoice-*subcommand) used to crash withError: Cannot read properties of undefined (reading 'invoiceId')whenever the local accounting store had no matching invoice.Root cause was the
process.exitinterceptor insrc/legacy/legacy-cli.ts'smain(): it scheduled an async Sphere destroy andreturn undefined as never, so the caller's synchronous control flow continued pastprocess.exit(1)and dereferencedmatched[0]on an empty array. There are ~180process.exit(N)call sites across this file that all share the same footgun.Fix shape
Convert the wrapper to throw a module-scope
ExitSignalsentinel synchronously when a Sphere instance is loaded. The outer try/catch inmain()detectsExitSignal, awaitscloseSphere(), and forwards the code through the original (non-wrapped)process.exitso the catch isn't re-entered. When no Sphere instance is loaded (early arg-validation / help paths), the wrapper falls straight through tooriginalExit, preserving prior synchronous behaviour.ExitSignaldeliberately doesn't extendErrorso innercatch (err)blocks filtering onerr instanceof Errordon't classify it as a normal error worth logging. Every inner catch in this file was audited — each either re-callsprocess.exit(N)(re-throws ExitSignal, propagates correctly) or sits over a try body that does noprocess.exit(so ExitSignal can't reach it).The prior #226 fix (explicit
returnafterprocess.exit(1)in invoice-deliver) is unreachable now but left in place as a defensive marker — touching it would widen this PR's scope.Verification
tsup) ✓, typecheck (tsc --noEmit) ✓, lint (eslint) clean (0 new errors).integrationSkip).invoice close/cancel/pay.Test plan
npx tsupbuilds cleannpm run typecheckcleannpx eslint src/legacy/legacy-cli.tsno new errorsSKIP_INTEGRATION=1 npx vitest run --config vitest.integration.config.ts test/integration/cli-invoice.integration.test.ts— 24 passedsphere invoice status 00005eb450a21d54...exits 1 withNo invoice found matching prefix: …and noCannot read properties of undefinedinvoice close/cancel/paywith bogus prefixinvoice liston empty wallet exits 0; arg-validation paths (missing positional) still exit 1 withUsage: …npm run test:integration)Related
sphere daemon start --detachexits immediately, leaving a stale PID file #19 / fix(daemon)(#19): keep --detach child alive past process.disconnect() #20 — daemon detach fix; surfaced this bug indirectly by lettingmanual-test-full-recovery.shreach §C.4 where it was triggered. Closed.