Skip to content

fix: make server.registerOperation() from components reachable via the ops API (#1736) - #1743

Merged
kriszyp merged 3 commits into
mainfrom
kris/1736-register-operation-cross-thread
Jul 10, 2026
Merged

fix: make server.registerOperation() from components reachable via the ops API (#1736)#1743
kriszyp merged 3 commits into
mainfrom
kris/1736-register-operation-cross-thread

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 9, 2026

Copy link
Copy Markdown
Member

Fixes #1736.

Problem

server.registerOperation() from a component's resources.js (or Plugin API handleApplication) — the standard, documented pattern — was permanently unreachable via the ops API. Components load per worker thread, so the registration lands in that worker's module-local OPERATION_FUNCTION_MAP; the ops-API HTTP dispatcher runs only on the main thread and reads the main thread's own instance of that map. Every caller, including super_user, got 400 "Operation '<name>' not found". Only the deprecated startOnMainThread extension hook worked.

Fix: a cross-thread bridge (server/serverHelpers/registeredOperations.ts)

Mirrors the existing RESOURCE_OPENAPI ITC request/response pattern, with one deliberate difference: executing an operation is side-effecting, so a request is forwarded to exactly one registering worker — never broadcast-first-wins.

  • Announce: a worker-side registerOperation() still sets its local map, then broadcasts OPERATION_REGISTERED; the main thread records name → Set<threadId>.
  • Forward on miss: when main-thread dispatch misses its local map, it checks the registry and forwards the request body (OPERATION_EXECUTE_REQUEST) to one live registering worker — rotating across workers, pruning dead ids — and awaits the requestId-correlated response. Unknown names still fail fast with 400 (the registry gates forwarding, so no timeout is paid for genuinely unknown operations).
  • Auth runs on the worker: the main thread authenticates and forwards the resolved hdb_user; the worker executes through the normal chooseOperation (full verifyPerms, where the operation function and its metadata actually exist) + processLocalTransaction. Workers never re-forward, so an unknown op can't loop. bypass_auth is re-stripped on the worker as defense-in-depth, and response correlation verifies the responding thread is the one the request was sent to.
  • Failure handling: worker death mid-flight rejects in-flight forwards with 503 and forgets that worker's registrations (onThreadExit hook added to manageThreads); a wedged worker is bounded by the ops-API network timeout; structured-clone failures report a clear error in both directions instead of an opaque hang.

This fixes both component contexts named in the issue investigation (jsResource and handleApplication) at the registerOperation layer, and as a side effect lets server.operation() on the main thread reach component-registered operations too. Worker-local calls (server.operation() in a worker) are untouched — they hit the local map as before.

Explicitly out of scope (follow-up: #1742)

  • Streaming results: a forwarded op returning a Readable gets an explicit 501 rather than an opaque failure (needs MessagePort transfer plumbing).
  • isJob: was already ignored by registerOperation() before this change (never wired to job_operation_function); unchanged.

Testing

  • New integration suite integrationTests/components/registered-operation.test.ts (fixture registers ops from resources.js, 2 worker threads): reachable via ops API with worker-side execution and forwarded user identity; repeated calls; error statusCode propagation (422); stream → explicit 501; unknown op → fast 400. 5/5 pass.
  • Regression smoke: authentication, delete, job-queue (97/97), components.test.mjs (25/25).
  • Build, oxlint, prettier clean.

Cross-model review (thorough: Gemini + Codex + domain pass)

No confirmed blockers. Adjudicated notes for reviewers:

  • Rolling-restart window (significant, accepted): a draining worker stays in the registry until its port closes, so a fresh forwarded op can land on a worker that's shutting down. If the worker exits mid-execution the forward rejects with 503 rather than hanging — the residual exposure (op partially executes, then 503) is inherent to any worker-crash window, not new to this path.
  • Post-deploy registration race (accepted): the announce is fire-and-forget, so a call arriving in the instant between component load and the main thread recording the announcement gets a transient not found — eventual-consistency window matching component load semantics generally.
  • Response correlation now includes an originator check (defense-in-depth, adopted from review).

🤖 Generated with Claude Code

— KrAIs (Claude Opus 4.8), for Kris

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements a cross-thread bridge for server.registerOperation(), allowing operations registered on worker threads to be reachable by the main-thread ops-API dispatcher. The feedback identifies two key issues: a potential infinite recursion bug in executeRemoteOperation due to parameter shadowing of the Promise resolve/reject functions, and an overly restrictive deletion of body.bypass_auth on the worker thread that could break legitimate internal use cases.

Comment thread server/serverHelpers/registeredOperations.ts Outdated
Comment thread server/serverHelpers/registeredOperations.ts Outdated
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Comment thread server/threads/manageThreads.js Outdated
Comment thread server/serverHelpers/registeredOperations.ts
Comment thread server/serverHelpers/registeredOperations.ts Outdated
Comment thread server/serverHelpers/registeredOperations.ts
kriszyp pushed a commit that referenced this pull request Jul 9, 2026
- Preserve bypass_auth as forwarded to a worker instead of stripping it:
  external HTTP requests already have it stripped upstream
  (handlePostRequest, before any dispatch decision runs), and an internal
  server.operation(op, context, false) caller must see the same
  authorize:false behavior whether the op runs locally or via the bridge.
  ITC is an internal, same-process trust boundary (gemini-code-assist).

- Destroy an abandoned streaming result before the explicit 501, since
  processLocalTransaction already ran the handler and the stream may hold
  an open fd/cursor/socket (cb1kenobi).

- Report a structured-clone failure on forward as 500, not 400 — it's a
  server-side limitation, not a malformed client request (cb1kenobi).

- Fix a worker-crash race where the REMOVE_PORT fast path (splice by
  threadId, no removePort() call) could beat this thread's own port
  'close'/'exit' event, leaving threadExitListeners never fired and an
  in-flight forward waiting out the full timeout instead of failing fast
  with 503. Route both removal paths through a shared, dedup-guarded
  notifyThreadExit (cb1kenobi).

- Rename the Promise executor's resolve/reject parameters to
  promiseResolve/promiseReject for clarity — verified (spec + compiled
  dist output + runtime test) that the original shorthand-method naming
  did not actually self-shadow, but the rename removes any doubt for
  future readers (gemini-code-assist).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

@cb1kenobi cb1kenobi left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Human and AI reviewed. No issues, neat feature!

Comment thread server/serverHelpers/registeredOperations.ts Outdated
Comment thread server/serverHelpers/registeredOperations.ts Outdated
Kris Zyp and others added 2 commits July 10, 2026 06:14
…e ops API (#1736)

Components (jsResource resources.js, Plugin API handleApplication) load
per-worker, so their registerOperation() calls landed in worker-local
OPERATION_FUNCTION_MAP instances that the main-thread ops-API dispatcher
never reads — the operation was unreachable for every caller.

Add a cross-thread bridge (mirroring the RESOURCE_OPENAPI ITC pattern,
but targeted at exactly one worker since execution is side-effecting):

- Workers announce each registration (OPERATION_REGISTERED); the main
  thread records name -> Set<threadId>.
- On a dispatch miss, the main thread forwards the request body
  (OPERATION_EXECUTE_REQUEST) to one live registering worker, rotating
  across workers and pruning dead ones, and awaits the correlated
  response with a timeout plus thread-exit rejection.
- The worker executes through the normal chooseOperation +
  processLocalTransaction path, so permission checks run where the
  operation function and its metadata exist, with the forwarded
  hdb_user; workers never re-forward, so unknown ops cannot loop.
- Streaming results are explicitly rejected (501) rather than failing
  opaquely; structured-clone failures report clearly in both directions.

Unknown operations still fail fast with 400 (the registry gates
forwarding, so no timeout is paid for genuinely unknown names).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Preserve bypass_auth as forwarded to a worker instead of stripping it:
  external HTTP requests already have it stripped upstream
  (handlePostRequest, before any dispatch decision runs), and an internal
  server.operation(op, context, false) caller must see the same
  authorize:false behavior whether the op runs locally or via the bridge.
  ITC is an internal, same-process trust boundary (gemini-code-assist).

- Destroy an abandoned streaming result before the explicit 501, since
  processLocalTransaction already ran the handler and the stream may hold
  an open fd/cursor/socket (cb1kenobi).

- Report a structured-clone failure on forward as 500, not 400 — it's a
  server-side limitation, not a malformed client request (cb1kenobi).

- Fix a worker-crash race where the REMOVE_PORT fast path (splice by
  threadId, no removePort() call) could beat this thread's own port
  'close'/'exit' event, leaving threadExitListeners never fired and an
  in-flight forward waiting out the full timeout instead of failing fast
  with 503. Route both removal paths through a shared, dedup-guarded
  notifyThreadExit (cb1kenobi).

- Rename the Promise executor's resolve/reject parameters to
  promiseResolve/promiseReject for clarity — verified (spec + compiled
  dist output + runtime test) that the original shorthand-method naming
  did not actually self-shadow, but the rename removes any doubt for
  future readers (gemini-code-assist).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kriszyp
kriszyp force-pushed the kris/1736-register-operation-cross-thread branch from dc52db3 to d7a2d0c Compare July 10, 2026 12:14
@kriszyp
kriszyp merged commit e4ad26a into main Jul 10, 2026
56 of 57 checks passed
@kriszyp
kriszyp deleted the kris/1736-register-operation-cross-thread branch July 10, 2026 12:27
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.

server.registerOperation() calls in a component's resources.js are unreachable via the ops API

2 participants