Skip to content

Log why request-handler unregistration fails during teardown#2573

Open
rolfheij-sil wants to merge 1 commit into
mainfrom
unregister-failure-logging
Open

Log why request-handler unregistration fails during teardown#2573
rolfheij-sil wants to merge 1 commit into
mainfrom
unregister-failure-logging

Conversation

@rolfheij-sil

@rolfheij-sil rolfheij-sil commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

At every app quit, the log shows an anonymous failure with zero context:

UnsubscriberAsyncList platformScriptureEditor: Unsubscriber at index 19 failed!

Diagnosis: the failing unsubscriber is a registerCommand unregistration that
resolves false rather than throwing, and nothing in the chain logs why.
The chain:

  • src/shared/services/network.service.ts — the unsubscriber closure returned
    by registerRequestHandler returns false if jsonRpc is unset, or passes
    through whatever jsonRpc.unregisterMethod(requestType) resolves.
  • src/client/services/rpc-client.ts unregisterMethod has three silent
    false-returning branches: the method isn't locally registered (checked
    twice, once before and once after acquiring the mutex), or the remote
    UNREGISTER_METHOD round-trip itself resolved false.

None of these branches logged anything, so UnsubscriberAsyncList only ever
saw an opaque false with no way to tell which branch fired or for which
request/method.

Changes

Added one logger.warn per silent branch, each naming the request/method
type so the log line is actionable:

  • network.service.ts: warn when the closure returns false because
    jsonRpc is unset, and warn if jsonRpc.unregisterMethod itself resolved
    false.
  • rpc-client.ts unregisterMethod: warn (matching this file's existing
    logger.warn convention for the analogous registerMethod "already
    registered" branches) distinguishing "not locally registered" from "remote
    unregister round-trip failed", each naming the methodName.

Behavior is unchanged — return values, ordering, and error propagation
are identical; this is purely additive logging. The underlying failure is
pre-existing and cosmetic (nothing depends on the unsubscriber succeeding at
quit-time); this PR just makes the next occurrence self-describing instead of
an anonymous index number.

Test plan

  • eslint on both changed files — clean
  • tsc -p ./tsconfig.json --noEmit — no new errors (the one pre-existing
    error, a missing generated release/app/buildInfo.json, reproduces
    identically on unmodified main and is unrelated to these files)
  • vitest run src/shared/services/__tests__/network.service.shared-events.test.ts
    — 10/10 passing (no dedicated unit tests exist for rpc-client.ts)

🤖 Generated with Claude Code

Self-review — 2026-07-17 (review-paratext methodology)

Approve as-is — clean, low-risk, additive-only diagnostic logging; no behavior change. Four analysis passes (api / style / compliance / ux) per .claude/agents/review-analyzer.md, run by a review-lead agent; author interview substituted with the PR body + PRD design records; every finding adversarially verified against code and tests.

# severity finding disposition
1 minor unregisterMethod's pre-mutex fast-path and post-mutex re-check log the identical message, so a log line alone can't distinguish "already gone" from "lost the race" accepted (mirrors existing registerMethod precedent)

Investigated separately: the "could this spam every shutdown" concern is unfounded — the new warn logs only fire on paths where two more-severe, pre-existing logs (console.error in UnsubscriberAsyncList, logger.error in deactivateExtension) already fire today.

Gates: prettier --check pass, tsc --noEmit 0 errors, eslint 0 errors/warnings, vitest network.service.shared-events.test.ts 10/10 pass.


This change is Reviewable

At app quit, UnsubscriberAsyncList logs a bare "Unsubscriber at index N
failed!" with no context when a registerCommand unregistration resolves
false instead of throwing. Add logger.warn calls at each silent
false-returning branch (jsonRpc unset in the network.service closure;
method not locally registered or remote UNREGISTER_METHOD round-trip
failed in rpc-client's unregisterMethod) so the next occurrence
self-describes. No behavior change — return values, ordering, and error
propagation are identical.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rolfheij-sil
rolfheij-sil force-pushed the unregister-failure-logging branch from fb6f3f0 to 105fbb4 Compare July 17, 2026 15:40
@lyonsil

lyonsil commented Jul 18, 2026

Copy link
Copy Markdown
Member

Code review

Found 1 issue:

  1. The new logger.warn on the !jsonRpc branch fires on every graceful shutdown, which is a documented, expected no-op rather than a failure. shutdown() tears down the handler reference first so disposers "skip the now-pointless per-event unregister call — the whole connection is already going away," and the sibling disposeNetworkEventEmitter returns silently for the same !jsonRpc condition. Warning here adds spurious noise on normal quit, working against the PR's goal of making teardown logs meaningful. Consider debug level (or no log) for this branch, reserving warn for the genuine unregistered === false case just below it.

return async () => {
if (!jsonRpc) {
logger.warn(`Could not unregister request handler for "${requestType}": jsonRpc is not set`);
return false;
}
removeTimeoutMsForRequestType(requestType);
const unregistered = await jsonRpc.unregisterMethod(requestType);
if (!unregistered) logger.warn(`Failed to unregister request handler for "${requestType}"`);
return unregistered;

Documented expected-quiet-shutdown intent it contradicts:

await jsonRpc.disconnect();
// Tear down the handler reference before disposing emitters so their disposers skip the
// now-pointless per-event unregister call — the whole connection is already going away.
jsonRpc = undefined;

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants