-
Notifications
You must be signed in to change notification settings - Fork 0
feat: ETag conditional requests for version checks #21
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| -- AlterTable | ||
| ALTER TABLE "SystemSettings" ADD COLUMN "latestServerReleaseEtag" TEXT, | ||
| ADD COLUMN "latestAgentReleaseEtag" TEXT, | ||
| ADD COLUMN "latestDevAgentReleaseEtag" TEXT; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,7 +3,7 @@ import { prisma } from "@/lib/prisma"; | |||||||||||||||||
| const GITHUB_API = "https://api.github.com"; | ||||||||||||||||||
| const SERVER_REPO = "TerrifiedBug/vectorflow"; | ||||||||||||||||||
| const AGENT_REPO = "TerrifiedBug/vectorflow"; | ||||||||||||||||||
| const CHECK_INTERVAL_MS = 24 * 60 * 60 * 1000; // 24 hours | ||||||||||||||||||
| const CHECK_INTERVAL_MS = 15 * 60 * 1000; // 15 minutes (ETag keeps most checks free) | ||||||||||||||||||
|
|
||||||||||||||||||
| interface GitHubRelease { | ||||||||||||||||||
| tag_name: string; | ||||||||||||||||||
|
|
@@ -20,18 +20,33 @@ interface GitHubReleaseWithAssets extends GitHubRelease { | |||||||||||||||||
| assets: GitHubAsset[]; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| async function fetchLatestRelease( | ||||||||||||||||||
| repo: string, | ||||||||||||||||||
| ): Promise<GitHubReleaseWithAssets | null> { | ||||||||||||||||||
| interface FetchResult { | ||||||||||||||||||
| release: GitHubReleaseWithAssets | null; | ||||||||||||||||||
| etag: string | null; | ||||||||||||||||||
| notModified: boolean; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| async function fetchRelease( | ||||||||||||||||||
| url: string, | ||||||||||||||||||
| etag?: string | null, | ||||||||||||||||||
| ): Promise<FetchResult> { | ||||||||||||||||||
| try { | ||||||||||||||||||
| const res = await fetch(`${GITHUB_API}/repos/${repo}/releases/latest`, { | ||||||||||||||||||
| headers: { Accept: "application/vnd.github.v3+json" }, | ||||||||||||||||||
| next: { revalidate: 0 }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| if (!res.ok) return null; | ||||||||||||||||||
| return res.json(); | ||||||||||||||||||
| const headers: Record<string, string> = { | ||||||||||||||||||
| Accept: "application/vnd.github.v3+json", | ||||||||||||||||||
| }; | ||||||||||||||||||
| if (etag) { | ||||||||||||||||||
| headers["If-None-Match"] = etag; | ||||||||||||||||||
| } | ||||||||||||||||||
| const res = await fetch(url, { headers, next: { revalidate: 0 } }); | ||||||||||||||||||
| if (res.status === 304) { | ||||||||||||||||||
| return { release: null, etag: etag ?? null, notModified: true }; | ||||||||||||||||||
| } | ||||||||||||||||||
| if (!res.ok) return { release: null, etag: null, notModified: false }; | ||||||||||||||||||
| const newEtag = res.headers.get("etag"); | ||||||||||||||||||
| const release = await res.json(); | ||||||||||||||||||
| return { release, etag: newEtag, notModified: false }; | ||||||||||||||||||
| } catch { | ||||||||||||||||||
| return null; | ||||||||||||||||||
| return { release: null, etag: null, notModified: false }; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -81,21 +96,34 @@ export async function checkServerVersion(force = false): Promise<{ | |||||||||||||||||
| let checkedAt: Date | null = lastChecked ?? null; | ||||||||||||||||||
|
|
||||||||||||||||||
| if (needsCheck) { | ||||||||||||||||||
| const release = await fetchLatestRelease(SERVER_REPO); | ||||||||||||||||||
| if (release) { | ||||||||||||||||||
| const { release, etag, notModified } = await fetchRelease( | ||||||||||||||||||
| `${GITHUB_API}/repos/${SERVER_REPO}/releases/latest`, | ||||||||||||||||||
| settings?.latestServerReleaseEtag, | ||||||||||||||||||
| ); | ||||||||||||||||||
|
Comment on lines
+99
to
+102
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When This directly contradicts the expected behaviour described in the PR's own test plan: "Force check — verify it fetches fresh data." A user hitting "Force Check" on the Settings page would silently receive stale data if nothing changed on GitHub's side. The same problem exists in The fix is to pass
Suggested change
Apply the same pattern in the other two functions. Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server/services/version-check.ts
Line: 99-102
Comment:
`force` flag does not bypass ETag — forced checks can still return cached data
When `force = true`, `needsCheck` is forced to `true`, but the stored ETag is still passed to `fetchRelease`. This means GitHub can still respond with `304 Not Modified`, and the function returns the previously-cached version instead of freshly fetched data.
This directly contradicts the expected behaviour described in the PR's own test plan: _"Force check — verify it fetches fresh data."_ A user hitting "Force Check" on the Settings page would silently receive stale data if nothing changed on GitHub's side.
The same problem exists in `checkAgentVersion` (lines 169–172) and `checkDevAgentVersion` (lines 246–249).
The fix is to pass `null` for the ETag when `force` is `true`:
```suggestion
const { release, etag, notModified } = await fetchRelease(
`${GITHUB_API}/repos/${SERVER_REPO}/releases/latest`,
force ? null : settings?.latestServerReleaseEtag,
);
```
Apply the same pattern in the other two functions.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||
| checkedAt = new Date(); | ||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Misleading
In the old code, The same pattern exists on lines 173 and 250 in Consider only setting Prompt To Fix With AIThis is a comment left during a code review.
Path: src/server/services/version-check.ts
Line: 103
Comment:
Misleading `checkedAt` on API failure
`checkedAt` is unconditionally set to `new Date()` before the API response is evaluated. If the API returns an error (not 304, not 200), neither the `notModified` nor the `release` branch fires, so nothing is written to the DB. The next call will correctly retry (the DB timestamp was never bumped), but the function still *returns* `checkedAt = new Date()`, telling the UI "last checked: just now" when the check actually failed silently.
In the old code, `checkedAt = new Date()` was set only inside the success branches, so on failure the UI would show the last successful check time instead.
The same pattern exists on lines 173 and 250 in `checkAgentVersion` and `checkDevAgentVersion`.
Consider only setting `checkedAt = new Date()` inside the branches that actually persist to the DB, or tracking a separate `checkFailed` state.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||
|
|
||||||||||||||||||
| if (notModified) { | ||||||||||||||||||
| // Nothing changed — just bump the check timestamp | ||||||||||||||||||
| await prisma.systemSettings.upsert({ | ||||||||||||||||||
| where: { id: "singleton" }, | ||||||||||||||||||
| update: { latestServerReleaseCheckedAt: checkedAt }, | ||||||||||||||||||
| create: { id: "singleton", latestServerReleaseCheckedAt: checkedAt }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| } else if (release) { | ||||||||||||||||||
| latestVersion = release.tag_name.replace(/^v/, ""); | ||||||||||||||||||
| releaseUrl = release.html_url; | ||||||||||||||||||
| checkedAt = new Date(); | ||||||||||||||||||
| await prisma.systemSettings.upsert({ | ||||||||||||||||||
| where: { id: "singleton" }, | ||||||||||||||||||
| update: { | ||||||||||||||||||
| latestServerRelease: latestVersion, | ||||||||||||||||||
| latestServerReleaseCheckedAt: checkedAt, | ||||||||||||||||||
| latestServerReleaseEtag: etag, | ||||||||||||||||||
| }, | ||||||||||||||||||
| create: { | ||||||||||||||||||
| id: "singleton", | ||||||||||||||||||
| latestServerRelease: latestVersion, | ||||||||||||||||||
| latestServerReleaseCheckedAt: checkedAt, | ||||||||||||||||||
| latestServerReleaseEtag: etag, | ||||||||||||||||||
| }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -138,22 +166,38 @@ export async function checkAgentVersion(force = false): Promise<{ | |||||||||||||||||
| let checkedAt: Date | null = lastChecked ?? null; | ||||||||||||||||||
|
|
||||||||||||||||||
| if (needsCheck) { | ||||||||||||||||||
| const release = await fetchLatestRelease(AGENT_REPO); | ||||||||||||||||||
| if (release) { | ||||||||||||||||||
| const { release, etag, notModified } = await fetchRelease( | ||||||||||||||||||
| `${GITHUB_API}/repos/${AGENT_REPO}/releases/latest`, | ||||||||||||||||||
| settings?.latestAgentReleaseEtag, | ||||||||||||||||||
| ); | ||||||||||||||||||
| checkedAt = new Date(); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (notModified) { | ||||||||||||||||||
| await prisma.systemSettings.upsert({ | ||||||||||||||||||
| where: { id: "singleton" }, | ||||||||||||||||||
| update: { latestAgentReleaseCheckedAt: checkedAt }, | ||||||||||||||||||
| create: { id: "singleton", latestAgentReleaseCheckedAt: checkedAt }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| // Return cached checksums | ||||||||||||||||||
| if (settings?.latestAgentChecksums) { | ||||||||||||||||||
| try { checksums = JSON.parse(settings.latestAgentChecksums); } catch { /* ignore */ } | ||||||||||||||||||
| } | ||||||||||||||||||
| } else if (release) { | ||||||||||||||||||
| latestVersion = release.tag_name.replace(/^v/, ""); | ||||||||||||||||||
| checksums = await fetchChecksums(release); | ||||||||||||||||||
| checkedAt = new Date(); | ||||||||||||||||||
| await prisma.systemSettings.upsert({ | ||||||||||||||||||
| where: { id: "singleton" }, | ||||||||||||||||||
| update: { | ||||||||||||||||||
| latestAgentRelease: latestVersion, | ||||||||||||||||||
| latestAgentReleaseCheckedAt: checkedAt, | ||||||||||||||||||
| latestAgentReleaseEtag: etag, | ||||||||||||||||||
| latestAgentChecksums: JSON.stringify(checksums), | ||||||||||||||||||
| }, | ||||||||||||||||||
| create: { | ||||||||||||||||||
| id: "singleton", | ||||||||||||||||||
| latestAgentRelease: latestVersion, | ||||||||||||||||||
| latestAgentReleaseCheckedAt: checkedAt, | ||||||||||||||||||
| latestAgentReleaseEtag: etag, | ||||||||||||||||||
| latestAgentChecksums: JSON.stringify(checksums), | ||||||||||||||||||
| }, | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
@@ -165,22 +209,6 @@ export async function checkAgentVersion(force = false): Promise<{ | |||||||||||||||||
| return { latestVersion, checksums, checkedAt }; | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| async function fetchDevRelease(): Promise<GitHubReleaseWithAssets | null> { | ||||||||||||||||||
| try { | ||||||||||||||||||
| const res = await fetch( | ||||||||||||||||||
| `${GITHUB_API}/repos/${AGENT_REPO}/releases/tags/dev`, | ||||||||||||||||||
| { | ||||||||||||||||||
| headers: { Accept: "application/vnd.github.v3+json" }, | ||||||||||||||||||
| next: { revalidate: 0 }, | ||||||||||||||||||
| }, | ||||||||||||||||||
| ); | ||||||||||||||||||
| if (!res.ok) return null; | ||||||||||||||||||
| return res.json(); | ||||||||||||||||||
| } catch { | ||||||||||||||||||
| return null; | ||||||||||||||||||
| } | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| async function fetchDevVersionString( | ||||||||||||||||||
| release: GitHubReleaseWithAssets, | ||||||||||||||||||
| ): Promise<string | null> { | ||||||||||||||||||
|
|
@@ -215,9 +243,22 @@ export async function checkDevAgentVersion(force = false): Promise<{ | |||||||||||||||||
| let checkedAt: Date | null = lastChecked ?? null; | ||||||||||||||||||
|
|
||||||||||||||||||
| if (needsCheck) { | ||||||||||||||||||
| const release = await fetchDevRelease(); | ||||||||||||||||||
| if (release) { | ||||||||||||||||||
| checkedAt = new Date(); | ||||||||||||||||||
| const { release, etag, notModified } = await fetchRelease( | ||||||||||||||||||
| `${GITHUB_API}/repos/${AGENT_REPO}/releases/tags/dev`, | ||||||||||||||||||
| settings?.latestDevAgentReleaseEtag, | ||||||||||||||||||
| ); | ||||||||||||||||||
| checkedAt = new Date(); | ||||||||||||||||||
|
|
||||||||||||||||||
| if (notModified) { | ||||||||||||||||||
| await prisma.systemSettings.upsert({ | ||||||||||||||||||
| where: { id: "singleton" }, | ||||||||||||||||||
| update: { latestDevAgentReleaseCheckedAt: checkedAt }, | ||||||||||||||||||
| create: { id: "singleton", latestDevAgentReleaseCheckedAt: checkedAt }, | ||||||||||||||||||
| }); | ||||||||||||||||||
| if (settings?.latestDevAgentChecksums) { | ||||||||||||||||||
| try { checksums = JSON.parse(settings.latestDevAgentChecksums); } catch { /* ignore */ } | ||||||||||||||||||
| } | ||||||||||||||||||
| } else if (release) { | ||||||||||||||||||
| const versionString = await fetchDevVersionString(release); | ||||||||||||||||||
| if (versionString) { | ||||||||||||||||||
| latestVersion = versionString; | ||||||||||||||||||
|
|
@@ -228,12 +269,14 @@ export async function checkDevAgentVersion(force = false): Promise<{ | |||||||||||||||||
| update: { | ||||||||||||||||||
| latestDevAgentRelease: latestVersion, | ||||||||||||||||||
| latestDevAgentReleaseCheckedAt: checkedAt, | ||||||||||||||||||
| latestDevAgentReleaseEtag: etag, | ||||||||||||||||||
| ...(versionString ? { latestDevAgentChecksums: JSON.stringify(checksums) } : {}), | ||||||||||||||||||
| }, | ||||||||||||||||||
| create: { | ||||||||||||||||||
| id: "singleton", | ||||||||||||||||||
| latestDevAgentRelease: latestVersion, | ||||||||||||||||||
| latestDevAgentReleaseCheckedAt: checkedAt, | ||||||||||||||||||
| latestDevAgentReleaseEtag: etag, | ||||||||||||||||||
| ...(versionString ? { latestDevAgentChecksums: JSON.stringify(checksums) } : {}), | ||||||||||||||||||
| }, | ||||||||||||||||||
| }); | ||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.