Skip to content

fix(frontend): handle orphaned api cleanup promise rejection - #53

Merged
dborup merged 4 commits into
masterfrom
codex/fix-app-api-orphaned-finally-rejection
Sep 15, 2026
Merged

dborup merged 4 commits into
masterfrom
codex/fix-app-api-orphaned-finally-rejection

Conversation

@dborup

@dborup dborup commented Sep 14, 2026

Copy link
Copy Markdown
Owner

Summary

public/app.js's api() helper does:

_inflight.set(path, promise);
promise.finally(() => _inflight.delete(path));
return promise;

.finally() returns its own derived promise that mirrors the outcome of the promise it's attached to. That derived promise was discarded uncaught, so when a request rejects, both the promise returned to callers (correctly awaited/caught by every real caller) and the orphaned .finally() promise reject — the second one with nothing ever attached to observe it. In a browser this only shows up as a benign Uncaught (in promise) console warning; it surfaced here because Node's stricter default (an unhandled rejection crashes the process) has no browser equivalent, while building a behavioral test for issue Kpa-clawbot#1375's scope-stats fetch caching.

Fix (one line, public/app.js):

promise.finally(() => _inflight.delete(path)).catch(() => {});

The added .catch() only consumes the discarded .finally() promise's mirrored rejection. It is a different promise object from the one returned to callers, so:

  • the original request's error still reaches the caller unchanged (proven by scenario B, matching the pre-fix error text exactly);
  • caching, TTL, in-flight dedup, and 503 retry behavior are all unchanged (scenarios D, E, F, G).

Commits in this PR

  1. d3c24830 — the production fix itself, plus the first version of the regression test.
  2. 86645756 — hardens the test after an independent review found two test-harness false positives (an incomplete-suite exit-0 path, and scenario C accepting an unrelated child crash as a pass).
  3. 98fbdbe2 — closes a further false positive in scenario H (a source-line echo containing the marker was mistaken for the real rejection) and fixes child-timeout labeling/SIGKILL handling, found in a second independent review round.
  4. 34061595 — registers the now-approved test into test-all.sh and the CI JS-tests step in deploy.yml, immediately before issue bug(analytics): Scopes tab fails JSON.parse — '/api/api/scope-stats' duplicate prefix (#915 fix never merged) Kpa-clawbot/CoreScope#1375's existing (currently failing, unrelated) test.

Evidence

  • Fail-before/pass-after: reproduced with node --unhandled-rejections=strict against a minimal harness loading the real, unmodified api() — before the fix, a caller correctly catches the rejection and the process still crashes afterward from the orphaned promise; after the fix, no crash.
  • Regression test (test-app-api-inflight-cleanup-rejection.js) covers 8 scenarios (A–H): success, unchanged failure semantics, no extra unhandled rejection (via a self-checking child process under --unhandled-rejections=strict), in-flight cleanup after both success and failure, concurrent-request dedup, TTL cache behavior, and a self-check that the strict-mode detection technique itself actually detects a deliberate unhandled rejection (and doesn't just pass on any unrelated crash).
  • Mutation testing (isolated scratch copies only, never the reviewed files): removing in-flight dedup, a typo'd api() reference, api() throwing before its fetch call, and reverting the production .catch() were each confirmed to produce a relevant, correctly-diagnosed test failure — not a vacuous pass or generic crash.
  • Watchdog / child-timeout hardening: a non-unref'd watchdog guards against an async stall silently exiting 0; both spawned child processes use SIGKILL on timeout with a clear timeout diagnosis rather than a generic spawn-error message. (This is an event-loop-based safety bound for async stalls — it cannot interrupt spawnSync or other synchronous blocking in the main process; an outer runner-level timeout remains the relevant backstop for that case.)
  • Independent review: the production fix and the final test harness (through 98fbdbe2) were both reviewed and approved in separate passes before the test was registered into CI.
  • CI registration: the new test is registered in both test-all.sh and deploy.yml's JS-tests step, placed immediately before issue bug(analytics): Scopes tab fails JSON.parse — '/api/api/scope-stats' duplicate prefix (#915 fix never merged) Kpa-clawbot/CoreScope#1375's existing test so it runs and is visible regardless of that unrelated, pre-existing failure.

Known baseline / out of scope

  • Issue bug(analytics): Scopes tab fails JSON.parse — '/api/api/scope-stats' duplicate prefix (#915 fix never merged) Kpa-clawbot/CoreScope#1375 (a separate, pre-existing stale-assertion test failure) is not fixed by this PR and is deliberately out of scope — it has its own separate branch/fix in progress.
  • test-frontend-helpers.js has 2 pre-existing favStar assertion failures on unmodified master, unrelated to this change.
  • test-live-dedup.js doesn't run in this environment (missing playwright dependency) — not a failure, just not run.
  • No staging or production verification has been performed for this change.
  • This fixes one specific orphaned-rejection source in api(). It does not claim that all unhandled rejections are eliminated, or that the rest of CI is green.

🤖 Generated with Claude Code

Dennis Jakobsen and others added 4 commits September 14, 2026 16:12
…le-rejecting

api() does `promise.finally(() => _inflight.delete(path))` and discards
the derived promise it returns. `.finally()` mirrors the outcome of the
promise it's attached to, so when a request rejects, that derived
promise rejects too, with nothing ever attached to observe it -- a
second, orphaned rejection alongside the one every real caller already
handles via the returned `promise`. Browsers only surface this as a
benign `Uncaught (in promise)` console warning; it was caught here
because Node's stricter default (an unhandled rejection crashes the
process) has no browser equivalent, and surfaced while building a
behavioral test for Kpa-clawbot#1375's scope-stats fetch caching.

Fix: attach a no-op `.catch(() => {})` to the discarded `.finally()`
promise. This only consumes that derived promise's mirrored rejection
-- the `promise` returned to callers is a different object and its
resolution/rejection to them is completely unaffected.

Reproduced fail-before / pass-after with `node --unhandled-rejections
=strict` against a minimal harness loading the real api(). New test
covers success, failure semantics, the no-extra-rejection fix itself
(via a child-process check under the same strict flag, with a negative
control proving that same check does catch a genuine unhandled
rejection), in-flight cleanup after both outcomes, retry-after-failure,
concurrent-request dedup, and TTL cache behavior -- all against the
real, unmodified api()/_apiCache/_inflight.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Round-2 (Opus) review of d3c2483 blocked test registration on two
harness bugs, both confirmed by reproduction before this fix and again
after it:

1. Broken in-flight dedup (removing `if (_inflight.has(path)) return
   _inflight.get(path);`) left scenario F's second Promise.all() branch
   permanently pending. Nothing else kept the event loop alive, so Node
   drained and exited 0 without ever reaching G/H or printing a summary
   -- a silent false pass. Fixed two ways: process.exitCode is now set
   to 1 immediately and only flipped to 0 after explicitly confirming
   all 8 named scenarios (A-H) ran to completion and passed; scenario F
   also now asserts exactly 1 fetch happened before resolving, catching
   this mutation instantly instead of relying on the fallback. A
   non-unref'd watchdog timer additionally guards against a genuine
   hang (a real pending macrotask keeps the event loop alive, so Node
   cannot silently exit early while it's armed) and is cleared on the
   normal completion path.

2. Scenario C's child script accepted ANY thrown error via a bare
   catch, so pointing it at a nonexistent `ctx.apiTypo` (or making
   api() throw before ever calling fetch) still printed a pass. The
   child now explicitly verifies api is a function, checks the exact
   rejection message, checks the exact fetch count and URL, and only
   then writes a unique success marker; the parent requires that exact
   marker plus a clean exit, no signal, and no stderr. Scenario H was
   narrowed to what it actually proves (the strict-mode child-process
   technique detects a deliberate unhandled rejection) and now checks
   for that rejection's specific message in stderr, so an unrelated
   child crash can no longer count as a passing negative control; its
   dead `exitCode = 0` line is removed.

Re-verified: 12/12 clean runs (~0.1s each, full A-H every time). Both
original false positives now fail correctly. Two new mutations checked
too: api() failing before its fetch call, and removing the production
fix's `.catch()` on the discarded `.finally()` promise -- the latter's
failure output names the exact same bug (`API 500: ...` at the
orphaned-rejection site), not a generic crash. No leftover processes
after any run. public/app.js is untouched (byte-identical to d3c2483).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Scenario H asserted only that stderr *contained* the marker string, but
when a child crashes for any reason (ReferenceError, SyntaxError, etc.)
Node echoes the offending source line -- which itself contains the
marker -- into stderr. That let H "pass" even for crashes unrelated to
the deliberate unhandled rejection the scenario exists to detect.
Replace it with an exact `^Error: <marker>$` (multiline) match against
the thrown-message line, plus explicit signal/status checks so a signal
kill (e.g. our own timeout) can never masquerade as the real crash.

Both C's and H's spawnSync calls used only the default SIGTERM on
timeout, which a misbehaving child can ignore and become orphaned, and
a timed-out call surfaced as a generic "failed to spawn: ETIMEDOUT"
message rather than a clear timeout. Add killSignal: 'SIGKILL' to both
and check `error.code === 'ETIMEDOUT'` before the generic spawn-error
assertion.

Finally, reword the watchdog's comment/message: it is an event-loop
safety bound for async stalls only, and firing does not mean "the loop
was otherwise alive" (it also fires when it's the sole handle); make
clear it cannot interrupt spawnSync or other synchronous blocking, so
an outer runner-level timeout remains the real backstop for that case.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds the already-reviewed test-app-api-inflight-cleanup-rejection.js
to test-all.sh and the deploy.yml "Run JS unit tests (packet-filter)"
step, so it actually runs in CI. No other lines changed in either file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@dborup
dborup merged commit 24760c3 into master Sep 15, 2026
5 of 6 checks passed
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.

1 participant