Skip to content

fix(env): forward tenant params on env patched emit so cards live-update (#1750)#1751

Open
Antonio-RiveroMartnez wants to merge 1 commit into
mainfrom
status-refresh
Open

fix(env): forward tenant params on env patched emit so cards live-update (#1750)#1751
Antonio-RiveroMartnez wants to merge 1 commit into
mainfrom
status-refresh

Conversation

@Antonio-RiveroMartnez

@Antonio-RiveroMartnez Antonio-RiveroMartnez commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Linked issue

Fixes #1750

Summary

  • Add a shared emitServiceEvent(app, {...}) helper (apps/agor-daemon/src/utils/emit-service-event.ts) that emits a Feathers service event with a correctly-shaped HookContext (path + params), so the global realtime publish handler can scope it.
  • Use it for the branches patched emit in updateEnvironment (branches.ts), passing resolvedParams so the tenant is resolvable.
  • Tests: helper unit tests, a configureRealtimePublish test proving a manual emit routes to the tenant channel when the hook carries params.tenant (no ambient scope), and an updated branches regression test asserting the emitted hook shape.

Why / context

The env card spinner never cleared on status transitions (start→running, stop/nuke→stopped) until a manual page refresh.

updateEnvironment persists via this.patch(), an override that calls super.patch() and therefore bypasses Feathers' automatic event dispatch, so it emits the patched event manually. The emit passed no publish context.

The load-bearing detail (verified in installed Feathers source): transport-commons' publish listener passes the third emit argument through unchanged as the publish hook (@feathersjs/transport-commons channels/index.ts:67-71), and only synthesizes a fake { path, service, app, result } when that arg is absent. Automatic events, by contrast, emit the full HookContext (@feathersjs/feathers events.ts:18). So a manual emit that passes raw params — or nothing — leaves configureRealtimePublish without a usable context.path / context.params. Under multi_tenancy.mode: required_from_auth it then can't resolve the tenant channel and suppresses the event to service-only sockets; browsers never receive it.

This hits the terminal transitions users wait on, which for kind/executor env variants are applied by background paths outside any request scope — and, since #1737, outside any ambient tenant DB scope:

  • start→running — flipped by the health-monitor timer.
  • stop/nuke→stopped — the executor path early-returns at stopping; the terminal write lands from an async executor-completion path.

Because those paths have no ambient tenant scope, the tenant must come from the emitted hook's params. The helper supplies exactly that.

A hard refresh fixes it because the initial findAll re-reads status directly, bypassing websockets.

Note: an earlier revision of this PR passed resolvedParams as the raw third arg. That is not a valid publish context (Feathers does not wrap it into context.params), so it did not actually resolve the tenant and it dropped the fake hook's path. This revision fixes the shape via the helper. Thanks to the review that caught it.

Implementation notes

Validation / test plan

  • emit-service-event.test.ts — helper emits a HookContext-shaped third arg (path/params/method/id).
  • realtime-publish.test.ts — a branches patched event whose hook carries params.tenant routes to the tenant channel under required_from_auth without ambient scope (previously would be suppressed).
  • branches.test.ts — updated regression test asserts the emitted hook shape (path: 'branches', params.tenant). Verified it fails when the emit shape is reverted to raw params.
  • vitest run on the three files — 69 passed. tsc --noEmit (agor-daemon) — clean. Pre-commit hooks — passed.
  • Manual validation pending on a required_from_auth deployment: start/stop/nuke a kind env and confirm the card clears the spinner without a refresh.

Risks / rollout / rollback

  • Low risk. Adds a helper and a properly-shaped context to one emit; no schema/API/config changes. In mode: static behavior is unchanged (tenant resolves to the static id regardless).
  • Rollback: revert this commit.

Out of scope / follow-ups

  • Audit other manual service emits for the same context-less shape and migrate them to emitServiceEvent (branches.ts:743/965/975, register-hooks.ts, register-routes.ts, register-services.ts:299).
  • The reporter's secondary observation — a lag (~10 min) before the executor stop/nuke path writes stopped (status not derived from tracked-pid liveness). That reconciliation path is separate from this event-delivery fix.

@Antonio-RiveroMartnez Antonio-RiveroMartnez force-pushed the status-refresh branch 2 times, most recently from 2e33cb9 to 8dcbc94 Compare July 1, 2026 15:17
…pdate

The env card spinner never cleared on status transitions
(start->running, stop/nuke->stopped) until a manual page refresh.

updateEnvironment persists via this.patch(), which bypasses Feathers'
automatic event dispatch, so it emits the `patched` event manually. The
emit passed no publish context. Feathers' transport-commons passes the
third emit arg through UNCHANGED as the publish `hook` (and only
synthesizes a fake `{ path, service, app, result }` when it is absent),
so the global publish handler had no usable `context.path` /
`context.params`: under `mode: required_from_auth` it could not resolve
the tenant channel and suppressed the event to service-only sockets.
This hits background transitions hardest (health-monitor start->running,
executor stop/nuke->stopped) which fire outside any request scope and,
since #1737, outside any ambient tenant DB scope.

Add an emitServiceEvent() helper that builds a correctly-shaped
HookContext (path + params carrying the tenant) and use it for the env
`patched` emit, passing resolvedParams. Refresh worked because findAll
re-reads status directly, bypassing websockets.

Tests: helper unit tests; a configureRealtimePublish test proving a
manual emit routes to the tenant channel when the hook carries
params.tenant (no ambient scope); and an updated branches regression
test asserting the emitted hook shape.

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

Labels

ai-reviewed-gpt-5-5 Reviewed by GPT-5.5 AI reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Env card spinner never clears on status change (start→running, stop/nuke→stopped) until manual refresh — live status not applied client-side

2 participants