Skip to content

fix(admin): map onCallStreamEffect setup errors to HttpsErrors - #130

Open
detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-admin-map-oncallstreameffect-setup-errors-to-h-6ce42b
Open

detail-app[bot] wants to merge 1 commit into
mainfrom
detail/bug-fix/fix-admin-map-oncallstreameffect-setup-errors-to-h-6ce42b

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Closes #111

Bug

onCallStreamEffect (the streaming callable trigger in packages/admin/src/lib/functions/on-call-stream.ts) did not map setup-phase schema failures to typed HttpsErrors the way its sibling onCallEffect does. It ran the whole effect through run with the error channel cast to never, then unconditionally rethrew whatever raw error rejected the promise.

As a result, when a caller sent input that failed inputSchema, a raw SchemaError escaped the handler. The firebase-functions runtime collapses any non-HttpsError thrown by a callable to a generic HttpsError('internal', 'INTERNAL'), so the client lost both the correct code (invalid-argument) and the actionable decode message. Chunk-encoding failures against chunkSchema collapsed the same way — losing the developer-facing 'Failed to encode function output' message. The divergence was unintentional: the streaming wrapper landed just before the sibling's setup-error recovery contract, so the contract was never applied to it.

Fix

Mirrored onCallEffect's setup-error recovery in onCallStreamEffect, restricted to the boundary phases:

  • decode-input failures are wrapped as FunctionSetupError({ phase: 'decode-input' }) and recovered via defaultSetupErrorResponse to HttpsError('invalid-argument', <schema message>); the handler and stream never run on bad input.
  • encode-output failures (a chunk violating chunkSchema) are wrapped as FunctionSetupError({ phase: 'encode-output' }) and recovered to HttpsError('internal', 'Failed to encode function output').
  • Switched from run (error channel cast to never) to runExit, branching on Exit.isSuccess, squashing the cause with Cause.squash, and rethrowing — so an HttpsError rejection reaches firebase-functions intact and is serialized with its code/message rather than collapsed to internal.
  • Defect logging is now gated by !isExpectedRejection(error), so HttpsErrors and ErrorReporter.ignore-annotated errors are no longer logged as Defect in onCallStream.
  • Handler-raised FunctionSetupErrors still reach the caller unchanged (recovery is restricted to the boundaries, not wrapped around the handler), matching the sibling's contract.

To avoid duplication, extracted defaultSetupErrorResponse into a new internal module packages/admin/src/lib/functions/recover-callable-setup-error.ts (mirrors the existing recover-setup-error.ts @internal convention), now imported by both wrappers. The change to on-call.ts is a pure extraction — behavior is identical.

Testing

  • Unit tests (on-call-stream.spec.ts, 18 tests) — strengthened the previously-blind …rejects.toThrow() to assert instanceof HttpsError, code === 'invalid-argument', the decode message, and that the handler did not run; added a setup-error recovery block (decode message, chunk-encode → internal/'Failed to encode function output', mid-stream chunk keeps prior chunks, handler HttpsError verbatim, handler FunctionSetupError not recovered) and a defect-logging block mirroring report.spec.ts (4 cases). All pass.
  • Sibling regressionon-call.spec.ts and report.spec.ts pass unchanged (14 tests), confirming the defaultSetupErrorResponse extraction is behavior-preserving.
  • Whole-admin regressionpnpm vitest run packages/admin passes 79/79 across 8 files; no regressions in unrelated modules.
  • Build, lint, formatpnpm nx run-many -t build succeeds for 8 projects; eslint and prettier --check are clean on all changed files.
  • End-to-end wire probe (not versioned) — drove the same callable through the real firebase-functions@7.3.2 onCallHandler (production HTTP entry, ESM build for HttpsError identity) with mocked Express req/res. Post-fix: decode-input → HTTP 400 INVALID_ARGUMENT with the decode message; encode-output → HTTP 500 with 'Failed to encode function output'; handler HttpsError('permission-denied') → HTTP 403 verbatim; sibling onCallEffect still → 400 INVALID_ARGUMENT. Ran the same probe against the unmodified buggy baseline and reproduced the exact report (decode-input → 500 { message: 'INTERNAL', status: 'INTERNAL' }; encode-output message collapsed to 'INTERNAL'), confirming the probe catches the bug and the fix resolves it.
  • Emulator smoke (not versioned) — installed OpenJDK 17, added a temporary onCallStreamEffect callable to @example/backend, started firebase emulators:start --only functions, and hit it with curl: bad input ({ count: 'nope' }) → HTTP 400 INVALID_ARGUMENT Expected number at ["count"]; happy path ({ count: 3 }) → HTTP 200 with [{index:0},{index:1},{index:2}]; grep -c "Defect in onCallStream" over the emulator log returned 0 for the bad-input call. Temporary example changes reverted afterward; the example app is untouched in the final tree.

Notes

  • The admin typecheck nx target fails in this environment on the effect-firebase dependency package with 29 pre-existing errors unrelated to this change (e.g. firestore-service.js resolution). Verified identical on the unmodified baseline (git stash + re-run); admin's own lib typechecks cleanly via its build target.
  • An onSetupError option for onCallStreamEffect (full sibling parity for custom mid-stream chunk-encode recovery) is deliberately out of scope — it has non-trivial semantics for a chunk that fails partway through a stream and is not required to fix the reported wire-level collapse. Left to a future feature.

Automatic Fixes PRs can be configured here.

@detail-app
detail-app Bot requested a review from fwal as a code owner September 19, 2026 03:43
@github-actions github-actions Bot added 🐛 fix Something is broken or doesn't work properly 📦 admin labels Sep 19, 2026
@greptile-apps

greptile-apps Bot commented Sep 19, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge, with the streaming callable now matching the established callable error-handling contract.

Summary

This PR aligns streaming callable setup-error handling with the existing non-streaming callable contract.

  • Maps input decoding failures to HttpsError('invalid-argument', ...).
  • Maps chunk-encoding failures to a stable internal HttpsError.
  • Preserves handler-originated errors while suppressing defect logs for expected rejections.
  • Extracts the shared callable setup-error mapping into an internal helper.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Callable request] --> B{Input schema configured?}
  B -->|Yes| C[Decode request data]
  B -->|No| D[Use raw request]
  C -->|Schema failure| E[invalid-argument HttpsError]
  C -->|Success| F[Run stream handler]
  D --> F
  F --> G[Encode next chunk]
  G -->|Schema failure| H[internal HttpsError]
  G -->|Success| I[Send chunk]
  I --> J{More chunks?}
  J -->|Yes| G
  J -->|No| K[Return collected chunks]
  E --> L[Firebase callable serialization]
  H --> L
  F -->|Handler failure| M[Preserve original error]
  M --> L
Loading

Reviews (1) · Last reviewed commit: "fix(admin): map onCallStreamEffect setup..."

@fwal fwal added this to the 1.0 milestone Sep 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📦 admin 🐛 fix Something is broken or doesn't work properly

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Detail Bug] Callable streaming functions return generic INTERNAL errors for schema validation failures

1 participant