-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(events): sync endpoints dropped their WS events — rename/delete left every tab stale #1562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
paoloantinori
wants to merge
12
commits into
debpalash:main
Choose a base branch
from
paoloantinori:fix/event-bus-threadpool-emit
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
a6f008e
docs(backlog): investigate VRAM/lifecycle bug + durable-fix exploration
paoloantinori c2955db
chore(backlog): initialize Backlog.md project structure
paoloantinori b72436a
Merge remote-tracking branch 'upstream/main'
paoloantinori 214a859
Merge remote-tracking branch 'upstream/main'
paoloantinori 5229a95
chore: untrack local backlog/ task tracker (gitignored)
paoloantinori e4c1ef0
Merge remote-tracking branch 'upstream/main'
paoloantinori 9615cd5
fix(events): sync endpoints dropped their WS events — rename/delete l…
paoloantinori dcaed7c
docs(changelog): one-line Unreleased entry with issue ref + credit (G…
paoloantinori fd30a6c
fix(review): last-write-wins loaders + no sleep-polling in the emit test
paoloantinori 31d4db6
Merge remote-tracking branch 'upstream/main' into fix/event-bus-threa…
paoloantinori 030bc47
fix(lint): makeLoader can't call useCallback (rules-of-hooks) — gener…
paoloantinori be007e9
style(frontend): oxfmt useAppData (CI format check)
paoloantinori File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| import { describe, it, expect, vi } from 'vitest'; | ||
| import { retryInitialLoad } from '../utils/initialLoadRetry'; | ||
|
|
||
| // #1158 class: the initial data loads (profiles/history/…) ran exactly once | ||
| // after the backend became reachable. A transient failure on that single call | ||
| // (backend restarting, WS reconnect window, a 502 from the LAN gate) left the | ||
| // voices panel empty with no retry — which reads to users as "all my voices | ||
| // are gone". The load itself already keeps the previous list on later (WS) | ||
| // reloads; only the INITIAL load must retry until first success. | ||
|
|
||
| describe('retryInitialLoad (#1158 class)', () => { | ||
| it('retries a failing loader until it succeeds', async () => { | ||
| let calls = 0; | ||
| const loader = vi.fn(async () => { | ||
| calls += 1; | ||
| if (calls < 3) throw new Error('transient'); | ||
| }); | ||
| await retryInitialLoad(loader, { baseDelayMs: 1 }); | ||
| expect(calls).toBe(3); | ||
| }); | ||
|
|
||
| it('does not retry after first success', async () => { | ||
| const loader = vi.fn(async () => {}); | ||
| await retryInitialLoad(loader, { baseDelayMs: 1 }); | ||
| expect(loader).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('stops retrying when cancelled', async () => { | ||
| const loader = vi.fn(async () => { | ||
| throw new Error('down'); | ||
| }); | ||
| const opts = { baseDelayMs: 1 }; | ||
| const p = retryInitialLoad(loader, opts); | ||
| opts.cancelled = true; | ||
| await p; | ||
| // At most one attempt plus whatever was already in flight; the point is | ||
| // it terminates instead of looping forever after cancellation. | ||
| expect(loader.mock.calls.length).toBeLessThan(5); | ||
| }); | ||
|
|
||
| // #1158 wiring contract: the initial load must pass loaders that REJECT on | ||
| // failure. useAppData's loadProfiles-style loaders swallow errors by design | ||
| // (WS reloads keep the previous list); if the initial-load call site ever | ||
| // stops using { rethrow: true }, the retry helper resolves on attempt one | ||
| // and the fix is silently inert again (skeptic finding F1). | ||
| it('integration: useAppData wires rethrowing loaders into the retry', async () => { | ||
| const { readFileSync } = await import('node:fs'); | ||
| // jsdom replaces the global URL; Node's fileURLToPath needs a REAL node: | ||
| // URL instance, so build one from the specifier's pathname directly. | ||
| const nodeUrl = await import('node:url'); | ||
| const NodeURL = nodeUrl.URL; | ||
| const hookPath = new NodeURL('../hooks/useAppData.js', import.meta.url).pathname; | ||
| const src = readFileSync(hookPath, 'utf8'); | ||
| const initialBlock = src.slice(src.indexOf('retryInitialLoad(')); | ||
| for (const loader of [ | ||
| 'loadProfiles', | ||
| 'loadHistory', | ||
| 'loadDubHistory', | ||
| 'loadProjects', | ||
| 'loadExportHistory', | ||
| ]) { | ||
| expect(initialBlock).toContain(`${loader}({ rethrow: true })`); | ||
| } | ||
| // and the loader factory must actually rethrow when asked | ||
| expect(src).toContain('if (rethrow) throw e;'); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| /** | ||
| * retryInitialLoad — run an initial data load until first success. | ||
| * | ||
| * The INITIAL app load differs from later (WS-triggered) reloads: on reload a | ||
| * failure keeps the previous list, but on first load there IS no previous | ||
| * list, so a single transient failure left the panel empty forever and read | ||
| * to users as "my voices are gone" (#1158 class). Retry with backoff until | ||
| * the first success; afterwards the caller's normal keep-previous-list | ||
| * behavior takes over. | ||
| * | ||
| * `opts.cancelled` is the caller's mount-cancellation flag — set it to true | ||
| * and any pending wait resolves without another attempt. | ||
| */ | ||
| export async function retryInitialLoad(loader, opts = {}) { | ||
| const baseDelayMs = opts.baseDelayMs ?? 500; | ||
| const maxDelayMs = opts.maxDelayMs ?? 4000; | ||
| let delay = 0; // first attempt is immediate; only failures pay the backoff | ||
| for (;;) { | ||
| if (opts.cancelled) return; | ||
| try { | ||
| await loader(); | ||
| return; | ||
| } catch { | ||
| // transient — retry | ||
| } | ||
| await new Promise((r) => setTimeout(r, delay || baseDelayMs)); | ||
| delay = Math.min((delay || baseDelayMs) * 2, maxDelayMs); | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| """event_bus.emit must deliver from threadpool threads (sync FastAPI endpoints). | ||
|
|
||
| PUT /profiles/{id} (rename), DELETE /profiles/{id}, and DELETE .../consent are | ||
| sync endpoints, so Starlette runs their bodies in a threadpool worker with no | ||
| running event loop. event_bus.emit() used to call asyncio.get_running_loop() | ||
| and silently drop the event (DEBUG log only) from exactly those threads, which | ||
| reached users as "I renamed a voice and the list went stale / looked empty" | ||
| (the frontend only refetches the voice list on the WS "profiles" event). | ||
|
|
||
| The test drives the real failure shape: subscribe on the serving loop, call | ||
| emit() from a plain thread (as the threadpool does), and assert the event | ||
| arrives in the listener queue. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import asyncio | ||
| import json | ||
| import threading | ||
|
|
||
| import pytest | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def bus(): | ||
| """Resolve the module per test — binding it at collection lets another | ||
| suite's `sys.modules` rebinding make this exercise a different object.""" | ||
| return __import__("core.event_bus", fromlist=["emit"]) | ||
|
|
||
|
|
||
| def test_emit_from_thread_reaches_serving_loop(bus, tmp_path): | ||
| loop = asyncio.new_event_loop() | ||
| received: list[str] = [] | ||
| started = threading.Event() | ||
| done = threading.Event() | ||
|
|
||
| def run_loop(): | ||
| asyncio.set_event_loop(loop) | ||
| loop.run_until_complete(_serve(bus, received, started, done)) | ||
|
|
||
| t = threading.Thread(target=run_loop, name="test-serving-loop") | ||
| t.start() | ||
| try: | ||
| assert started.wait(2.0), "serving loop never subscribed" | ||
|
|
||
| # The bug's exact context: no running loop in this thread, like a | ||
| # Starlette threadpool worker executing a sync endpoint body. | ||
| def sync_endpoint_body(): | ||
| bus.emit("profiles", {"action": "updated", "id": "abc123"}) | ||
|
|
||
| worker = threading.Thread(target=sync_endpoint_body, name="threadpool-worker") | ||
| worker.start() | ||
| worker.join(2.0) | ||
|
|
||
| assert done.wait(2.0), ( | ||
| "emit() from a threadpool thread was dropped — the WS 'profiles' " | ||
| "event never reached the serving loop's listener queue" | ||
| ) | ||
| payload = json.loads(received[0]) | ||
| assert payload["kind"] == "profiles" | ||
| assert payload["action"] == "updated" | ||
| assert payload["id"] == "abc123" | ||
| finally: | ||
| loop.call_soon_threadsafe(done.set) | ||
| t.join(2.0) | ||
| loop.close() | ||
|
|
||
|
|
||
| async def _serve(bus, received: list[str], started: threading.Event, done: threading.Event): | ||
| q = await bus.subscribe() | ||
| started.set() | ||
| # Await the event itself (no sleep-polling): a failure surfaces as | ||
| # asyncio.TimeoutError, which fails the test with a clear traceback. | ||
| try: | ||
| received.append(await asyncio.wait_for(q.get(), 2.0)) | ||
| finally: | ||
| done.set() | ||
| await bus.unsubscribe(q) |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.