GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — [connectors] Report "no Samply profile" as 204, not as an error by blp · Pull Request #7934 · feldera/feldera Conversation
|
| Request | Before | After |
|---|---|---|
?latest=true, profile in progress |
204 + Retry-After |
same |
?latest=true, no profile |
400 NoSamplyProfile |
204, no Retry-After |
no latest, no profile |
400 NoSamplyProfile |
same |
Retry-After as "no profile".Multihost needs feldera/cloud#2207: the coordinator read a host's 204 as an empty profile.
samply_profile_latest_without_profile engine test. It fails (400 instead of 204) with the fix disabled.svelte-check: 0 errors. Prettier: clean.-- Claude Code, at @blp's request.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
When the Samply tab opens, the web console checks for a profile with `GET /samply_profile?latest=true`. Before any profile, the pipeline answered 400 `NoSamplyProfile`, so each check put an error in the browser's network log. In feldera/cloud#2163, these errors sent an investigation of a failed support bundle the wrong way. With `latest=true`, a 204 No Content now means that there is no profile to download yet: with `Retry-After` if one is in progress, as before, and without it if none was started. Without `latest`, nothing changes. Also corrects the API docs, which said 307 for a profile in progress. Signed-off-by: Ben Pfaff <[email protected]>
| const expectedInSeconds = parseInt(response.headers.get('Retry-After') ?? '') | ||
| const retryAfter = response.headers.get('Retry-After') | ||
| if (retryAfter === null) { | ||
| return null |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
missing (medium): this new 204 branch is the client half of the contract, and no test covers it. The Rust test only checks the server. Here is a unit test I ran locally with vitest --project server (5/5 pass). It could go in as src/lib/services/samplyProfile.spec.ts:
import { afterEach, describe, expect, it, vi } from 'vitest' import { getSamplyProfile } from '#lib/services/pipelineManager.ts' const stubFetch = (response: Response) => vi.stubGlobal('fetch', Object.assign(vi.fn().mockResolvedValue(response), { preconnect: () => {} })) describe('getSamplyProfile', () => { afterEach(() => vi.unstubAllGlobals()) it('returns null on 204 without Retry-After', async () => { stubFetch(new Response(null, { status: 204 })) expect(await getSamplyProfile('p', true)).toBeNull() }) it('returns expectedInSeconds on 204 with Retry-After', async () => { stubFetch(new Response(null, { status: 204, headers: { 'Retry-After': '7' } })) expect(await getSamplyProfile('p', true)).toEqual({ expectedInSeconds: 7 }) }) it('returns expectedInSeconds 0 on 204 with Retry-After: 0', async () => { stubFetch(new Response(null, { status: 204, headers: { 'Retry-After': '0' } })) expect(await getSamplyProfile('p', true)).toEqual({ expectedInSeconds: 0 }) }) it('throws on 204 with a malformed Retry-After', async () => { stubFetch(new Response(null, { status: 204, headers: { 'Retry-After': 'soon' } })) await expect(getSamplyProfile('p', true)).rejects.toThrow(/estimated time is not known/) }) it('returns a download handle on 200', async () => { stubFetch(new Response(new Uint8Array([1, 2, 3]), { status: 200, headers: { 'Content-Type': 'application/gzip', 'Content-Disposition': 'attachment; filename="p.json.gz"' } })) const result = await getSamplyProfile('p', true) expect(result && 'downloadPromise' in result).toBe(true) }) })
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
What I ran
Gates: the server change has a test, the changelog marks the incompatible change, and the API docs are fixed in the Rust source. I found no flaky tests, unsafe code or dependency changes.
Otherwise LGTM. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.