Skip to content

Fix stale connection status reads (worker detail, overview, Emily)#2300

Open
federicodeponte wants to merge 6 commits into
mainfrom
fix/stale-connection-reads
Open

Fix stale connection status reads (worker detail, overview, Emily)#2300
federicodeponte wants to merge 6 commits into
mainfrom
fix/stale-connection-reads

Conversation

@federicodeponte

Copy link
Copy Markdown
Member

Summary

Federico's working theory in floomhq/workeros-cloud#1209 was correct: one shared root cause, not three separate bugs. This fixes the connection-status read path once instead of patching each surface.

  • Frontend: the shared TanStack Query cache (30s staleTime, refetchOnMount:false, persisted to localStorage so it survives even a brand-new tab) was never invalidated when a connection completed. Both live completion points now invalidate the connections, worker-detail, and system/overview query roots before routing the user back:
    • /connections/redirect's poll loop (the browse/connect flow; return_to can be a worker detail page, which is the exact floomhq/workeros-cloud#1209 repro path)
    • /connections/callback (the OAuth provider's actual redirect_uri, the other live return leg)
  • Backend (floomhq/workeros-cloud#1209 / floomhq/workeros-cloud#1206): _available_connection_slugs_for_users had a bare try/except Exception: return set(). A transient DB failure looked identical to "genuinely no connections" to every caller (worker detail's missing_connections, the grid's missing-connections banner, Overview's connected-apps read), so a DB hiccup on one surface flipped its connection status while a fraction of a second later a different, unaffected surface showed the real state. Now logs the exception so a real DB error is distinguishable from an empty result in observability. Still returns an empty set on failure (no established partial-failure sentinel exists on this response shape yet, and every caller already treats "unknown" and "none" the same way), so this is deliberately not a new error-surfacing contract across three call sites.
  • Backend (floomhq/workeros-cloud#1205): _build_capabilities_snapshot (baked into Emily's system prompt every turn) enumerated workers via a raw repos.workers.list(user_id=user_id) call: a THIRD worker-enumeration path alongside the grid and Emily's workers__list_all tool (both already route through list_operator_workers, the documented single source of truth from Worker count disagrees across workers_list (13), system_stats.total_workers (8), and workspace_chat/Emily ("8", missing real workers) #2270). The raw call passed no role, so it used a different, narrower visibility rule (owner-only, and it never excluded archived workers), which is exactly how Emily could describe workers/automations the grid does not show. Now routes through the same list_operator_workers call the grid and the tool use, with the same role resolution workers__list_all already uses.

Why one root cause explains all three issues

Federico's theory: "the connection-status read path (cache/endpoint) returns empty/stale on first load and different surfaces render different conclusions from it." Confirmed on both sides:

  • Frontend: no surface ever invalidated the shared cache after a connection mutation, so whichever surface you land on next renders whatever was cached before the mutation.
  • Backend: _available_connection_slugs_for_user(s) is the single function all three read surfaces (worker_serialize.py's detail missing_connections, worker_listing.py's grid banner, overview.py's connected-apps list) call, and it silently degraded to "empty" on any transient failure with zero observability.
  • floomhq/workeros-cloud#1205 is the same class of bug in a different read path: a third, uncoordinated worker-enumeration query that disagreed with the canonical one.

Test plan

  • apps/api: new tests/test_emily_capabilities_snapshot_worker_split_brain.py (3 tests: fresh workspace reports 0 workers even with seeded stock workers owned by another identity, snapshot count matches list_operator_workers exactly with seeded stock excluded, archived workers excluded). Existing tests/test_emily_worker_split_brain_round09.py still passes unchanged.
  • apps/web: new tests/connections-invalidate-stale-reads.test.ts (source-inspection, matching the existing pattern for this class of test in run-page-cache-invalidation.test.ts / emily-create-from-prompt-invalidation.test.ts, since jsdom + fake timers are documented as unreliable for this component's async poll loop). Updated the three existing DOM tests that render /connections/redirect and /connections/callback to wrap in a real QueryClientProvider (now required since both call useQueryClient).
  • npm run lint:emdash passes (0 hits).
  • npx eslint on all changed files: 0 errors.
  • Ran the wider apps/api connection/Emily/worker test surface (30 files, ~220 tests) and the wider apps/web connections/worker-detail test surface (9 files, 42 tests): all pass except 5 pre-existing failures confirmed to also fail identically on a clean origin/main worktree (test-order-dependent flakiness unrelated to this diff: test_connection_reconnect_dedupe.py, test_connection_reliability.py, test_worker_create_connection_preflight.py).

Scope note

This does not add a new partial-failure sentinel field to any API response. If Federico wants the frontend to visibly distinguish "we could not check your connections" from "you have no connections" (rather than just having it in logs), that is a small follow-up on top of this, not included here since no existing pattern for it exists in the codebase and the instructions were to fix the observability gap at minimum, not invent a new contract across three call sites.

Fixes floomhq/workeros-cloud#1209, floomhq/workeros-cloud#1205, floomhq/workeros-cloud#1206

Three surfaces (worker detail's Connect-the-tool banner, Overview's
connected-apps read, Emily's capabilities snapshot) could disagree about
whether a tool was connected, or show a connected tool as disconnected
right after the user connected it.

Frontend root cause: the shared TanStack Query cache (30s staleTime,
refetchOnMount:false, persisted to localStorage) was never invalidated
when a connection completed. Both live completion points now invalidate
the connections, worker-detail, and overview query roots before routing
the user back: the /connections/redirect poll loop, and the
/connections/callback OAuth return leg.

Backend root cause (#1209/#1206): a bare try/except in
_available_connection_slugs_for_users silently returned an empty set on
any DB error, indistinguishable from "genuinely no connections". Now
logs the exception so a real DB error is distinguishable in
observability from an empty result.

Backend root cause (#1205): _build_capabilities_snapshot (baked into
Emily's system prompt) enumerated workers via a raw
repos.workers.list(user_id=user_id) call, a third worker-enumeration
path alongside the grid and Emily's workers__list_all tool (both already
route through list_operator_workers, #2270). The raw call used a
different (owner-only, no archived filter) visibility rule, so Emily's
snapshot could describe workers/state the grid does not show. Now routes
through the same list_operator_workers path as the grid and the tool.

Fixes floomhq/workeros-cloud#1209, floomhq/workeros-cloud#1205, floomhq/workeros-cloud#1206

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@federicodeponte

Copy link
Copy Markdown
Member Author

Merge-queue FAIL resolved. The OAuth redirect and callback now call a shared connection-cache helper that uses invalidateQueries with refetchType: "all" for connections, worker detail, and system overview keys. This refetches inactive reads even with refetchOnMount: false. Added a behavioral QueryClient regression test that starts all three queries inactive and verifies each fetches again and receives revision 2. Merged current Floom main so this head also contains #2299 and the current engine commits. Local verification: 15 related tests passed with 1 documented skip, full web lint completed with zero errors, and production build passed. All seven remote checks are green on 7fb343bd9ea768275dd70c37627ec2dc886f961b.

@federicodeponte

Copy link
Copy Markdown
Member Author

Freshness recheck completed after Floom main advanced.

  • Merged current Floom main 8d512f63bac062f34af9d4999e8d18648c39c625 into the existing PR branch without conflicts.
  • The connection-status helper still invalidates with refetchType: "all", and the behavioral test still proves inactive queries with refetchOnMount: false refetch.
  • All seven remote checks are green on final head 1a71c04895ef16fecce23514886c261b4a5e364b.

The original inactive persisted-query blocker remains resolved on the current-main-containing head. This PR was not merged.

@federicodeponte

Copy link
Copy Markdown
Member Author

Final current-main refresh completed.

  • Merged Floom main a8e4208bea1df57b788dbc3e55fecf977261e643 into the existing branch.
  • The inactive persisted-query refetch implementation and behavioral regression remain intact.
  • All seven remote checks are green on head ebdcfef94ca8aad4b05c13cdb860044c5573756c.

The original blocker is resolved. This PR was not merged.

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