-
-
Notifications
You must be signed in to change notification settings - Fork 67
Delta manifest: reduce update response sizes by 75-85% #1430
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
base: main
Are you sure you want to change the base?
Changes from all commits
f3dcaeb
25e41f6
5d39d72
daa803a
f833853
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 |
|---|---|---|
|
|
@@ -286,7 +286,7 @@ export function getAlias() { | |
| return { versionAlias, channelDevicesAlias, channelAlias } | ||
| } | ||
|
|
||
| function getSchemaUpdatesAlias(includeMetadata = false) { | ||
| function getSchemaUpdatesAlias(includeMetadata = false, oldVersionId?: number | null) { | ||
| const { versionAlias, channelDevicesAlias, channelAlias } = getAlias() | ||
|
|
||
| const versionSelect: any = { | ||
|
|
@@ -322,7 +322,24 @@ function getSchemaUpdatesAlias(includeMetadata = false) { | |
| allow_device_self_set: channelAlias.allow_device_self_set, | ||
| public: channelAlias.public, | ||
| } | ||
| const manifestSelect = sql<{ file_name: string, file_hash: string, s3_path: string }[]>`COALESCE(json_agg( | ||
| // Delta manifest: when oldVersionId is provided, exclude files that exist identically in old version | ||
| const manifestSelect = oldVersionId | ||
| ? sql<{ file_name: string, file_hash: string, s3_path: string }[]>`COALESCE(json_agg( | ||
| json_build_object( | ||
| 'file_name', ${schema.manifest.file_name}, | ||
| 'file_hash', ${schema.manifest.file_hash}, | ||
| 's3_path', ${schema.manifest.s3_path} | ||
| ) | ||
| ) FILTER ( | ||
| WHERE ${schema.manifest.file_name} IS NOT NULL | ||
| AND NOT EXISTS ( | ||
| SELECT 1 FROM ${schema.manifest} AS old_m | ||
| WHERE old_m.app_version_id = ${oldVersionId} | ||
| AND old_m.file_name = ${schema.manifest.file_name} | ||
| AND old_m.file_hash = ${schema.manifest.file_hash} | ||
| ) | ||
| ), '[]'::json)` | ||
| : sql<{ file_name: string, file_hash: string, s3_path: string }[]>`COALESCE(json_agg( | ||
| json_build_object( | ||
| 'file_name', ${schema.manifest.file_name}, | ||
| 'file_hash', ${schema.manifest.file_hash}, | ||
|
|
@@ -339,8 +356,9 @@ export function requestInfosChannelDevicePostgres( | |
| drizzleClient: ReturnType<typeof getDrizzleClient>, | ||
| includeManifest: boolean, | ||
| includeMetadata = false, | ||
| oldVersionId?: number | null, | ||
| ) { | ||
| const { versionSelect, channelDevicesAlias, channelAlias, channelSelect, manifestSelect, versionAlias } = getSchemaUpdatesAlias(includeMetadata) | ||
| const { versionSelect, channelDevicesAlias, channelAlias, channelSelect, manifestSelect, versionAlias } = getSchemaUpdatesAlias(includeMetadata, oldVersionId) | ||
| const baseSelect = { | ||
| channel_devices: { | ||
| device_id: channelDevicesAlias.device_id, | ||
|
|
@@ -376,8 +394,9 @@ export function requestInfosChannelPostgres( | |
| drizzleClient: ReturnType<typeof getDrizzleClient>, | ||
| includeManifest: boolean, | ||
| includeMetadata = false, | ||
| oldVersionId?: number | null, | ||
| ) { | ||
| const { versionSelect, channelAlias, channelSelect, manifestSelect, versionAlias } = getSchemaUpdatesAlias(includeMetadata) | ||
| const { versionSelect, channelAlias, channelSelect, manifestSelect, versionAlias } = getSchemaUpdatesAlias(includeMetadata, oldVersionId) | ||
| const platformQuery = platform === 'android' ? channelAlias.android : platform === 'electron' ? channelAlias.electron : channelAlias.ios | ||
| const baseSelect = { | ||
| version: versionSelect, | ||
|
|
@@ -422,17 +441,18 @@ export function requestInfosPostgres( | |
| channelDeviceCount?: number | null, | ||
| manifestBundleCount?: number | null, | ||
| includeMetadata = false, | ||
| oldVersionId?: number | null, | ||
| ) { | ||
| const shouldQueryChannelOverride = channelDeviceCount === undefined || channelDeviceCount === null ? true : channelDeviceCount > 0 | ||
| const shouldFetchManifest = manifestBundleCount === undefined || manifestBundleCount === null ? true : manifestBundleCount > 0 | ||
|
|
||
| const channelDevice = shouldQueryChannelOverride | ||
| ? requestInfosChannelDevicePostgres(c, app_id, device_id, drizzleClient, shouldFetchManifest, includeMetadata) | ||
| ? requestInfosChannelDevicePostgres(c, app_id, device_id, drizzleClient, shouldFetchManifest, includeMetadata, oldVersionId) | ||
| : Promise.resolve(undefined).then(() => { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'Skipping channel device override query' }) | ||
| return null | ||
| }) | ||
| const channel = requestInfosChannelPostgres(c, platform, app_id, defaultChannel, drizzleClient, shouldFetchManifest, includeMetadata) | ||
| const channel = requestInfosChannelPostgres(c, platform, app_id, defaultChannel, drizzleClient, shouldFetchManifest, includeMetadata, oldVersionId) | ||
|
|
||
| return Promise.all([channelDevice, channel]) | ||
| .then(([channelOverride, channelData]) => ({ channelData, channelOverride })) | ||
|
|
@@ -509,6 +529,37 @@ export async function getAppVersionPostgres( | |
| } | ||
| } | ||
|
|
||
| export async function getVersionIdByName( | ||
| c: Context, | ||
| appId: string, | ||
| versionName: string, | ||
| drizzleClient: ReturnType<typeof getDrizzleClient>, | ||
| allowedDeleted?: boolean, | ||
| ): Promise<number | null> { | ||
|
Comment on lines
+532
to
+538
|
||
| try { | ||
| // Return null for internal version names | ||
| if (!versionName || versionName === 'builtin' || versionName === 'unknown') | ||
| return null | ||
|
|
||
| const result = await drizzleClient | ||
| .select({ id: schema.app_versions.id }) | ||
| .from(schema.app_versions) | ||
| .where(and( | ||
| eq(schema.app_versions.app_id, appId), | ||
| eq(schema.app_versions.name, versionName), | ||
| ...(allowedDeleted !== undefined ? [eq(schema.app_versions.deleted, allowedDeleted)] : []), | ||
| )) | ||
| .limit(1) | ||
| .then(data => data[0]) | ||
|
|
||
| return result?.id ?? null | ||
| } | ||
| catch (e: unknown) { | ||
| logPgError(c, 'getVersionIdByName', e) | ||
| return null | ||
| } | ||
| } | ||
|
Comment on lines
+532
to
+561
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. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find all calls to getVersionIdByName with context
rg -n 'getVersionIdByName\(' --type ts -A3 -B1Repository: Cap-go/capgo Length of output: 897 🏁 Script executed: #!/bin/bash
# Get full context of the getVersionIdByName call in update.ts
sed -n '150,190p' supabase/functions/_backend/utils/update.ts
# Find getAppVersionPostgres to compare implementation pattern
rg -n 'getAppVersionPostgres\(' --type ts -A20 | head -50Repository: Cap-go/capgo Length of output: 6761 🏁 Script executed: #!/bin/bash
# Check for comments around allowedDeleted parameter definitions
rg -n 'allowedDeleted' --type ts -B3 -A3 | head -80
# Search for any TODOs or comments about deleted versions in delta calculations
rg -n 'deleted.*delta|delta.*deleted' --type ts -iRepository: Cap-go/capgo Length of output: 3648 Consider defaulting The function is called without the While
|
||
|
|
||
| export async function getAppVersionsByAppIdPg( | ||
| c: Context, | ||
| appId: string, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,7 @@ import { getBundleUrl, getManifestUrl } from './downloadUrl.ts' | |
| import { simpleError200 } from './hono.ts' | ||
| import { cloudlog } from './logging.ts' | ||
| import { sendNotifOrg } from './notifications.ts' | ||
| import { closeClient, getAppOwnerPostgres, getDrizzleClient, getPgClient, requestInfosPostgres, setReplicationLagHeader } from './pg.ts' | ||
| import { closeClient, getAppOwnerPostgres, getDrizzleClient, getPgClient, getVersionIdByName, requestInfosPostgres, setReplicationLagHeader } from './pg.ts' | ||
| import { makeDevice } from './plugin_parser.ts' | ||
| import { s3 } from './s3.ts' | ||
| import { createStatsBandwidth, createStatsMau, createStatsVersion, onPremStats, sendStatsAndDevice } from './stats.ts' | ||
|
|
@@ -162,7 +162,21 @@ export async function updateWithPG( | |
| // Only query link/comment if plugin supports it (v5.35.0+, v6.35.0+, v7.35.0+, v8.35.0+) AND app has expose_metadata enabled | ||
| const needsMetadata = appOwner.expose_metadata && !isDeprecatedPluginVersion(pluginVersion, '5.35.0', '6.35.0', '7.35.0', '8.35.0') | ||
|
|
||
| const requestedInto = await requestInfosPostgres(c, platform, app_id, device_id, defaultChannel, drizzleClient, channelDeviceCount, manifestBundleCount, needsMetadata) | ||
| // Look up old version ID for delta manifest calculation | ||
| // Only do this if we're fetching manifest entries | ||
| let oldVersionId: number | null = null | ||
| if (fetchManifestEntries && version_name && version_name !== 'builtin' && version_name !== 'unknown') { | ||
|
||
| oldVersionId = await getVersionIdByName(c, app_id, version_name, drizzleClient) | ||
|
Comment on lines
+165
to
+169
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.
Because Useful? React with 👍 / 👎. |
||
| cloudlog({ | ||
| requestId: c.get('requestId'), | ||
| message: 'Delta manifest lookup', | ||
| version_name, | ||
| oldVersionId, | ||
| fetchManifestEntries, | ||
| }) | ||
| } | ||
|
|
||
| const requestedInto = await requestInfosPostgres(c, platform, app_id, device_id, defaultChannel, drizzleClient, channelDeviceCount, manifestBundleCount, needsMetadata, oldVersionId) | ||
| const { channelOverride } = requestedInto | ||
| let { channelData } = requestedInto | ||
| cloudlog({ requestId: c.get('requestId'), message: `channelData exists ? ${channelData !== undefined}, channelOverride exists ? ${channelOverride !== undefined}` }) | ||
|
|
@@ -185,6 +199,17 @@ export async function updateWithPG( | |
| const manifestEntries = (channelOverride?.manifestEntries ?? channelData?.manifestEntries ?? []) as Partial<Database['public']['Tables']['manifest']['Row']>[] | ||
| // device.version = versionData ? versionData.id : version.id | ||
|
|
||
| // Check if device is already on the latest version BEFORE checking for missing bundle | ||
| // This must come first because delta manifest calculation may return empty manifest | ||
| // when device is already on target version (all files match), which would incorrectly | ||
| // trigger the no_bundle error for manifest-only bundles | ||
| if (version_name === version.name) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'No new version available', id: device_id, version_name, version: version.name, date: new Date().toISOString() }) | ||
| // TODO: check why this event is send with wrong version_name | ||
| await sendStatsAndDevice(c, device, [{ action: 'noNew', versionName: version.name }]) | ||
| return simpleError200(c, 'no_new_version_available', 'No new version available') | ||
| } | ||
|
|
||
| // TODO: find better solution to check if device is from apple or google, currently not working in | ||
|
|
||
| if (!version.external_url && !version.r2_path && !isInternalVersionName(version.name) && (!manifestEntries || manifestEntries.length === 0)) { | ||
|
|
@@ -205,14 +230,6 @@ export async function updateWithPG( | |
| }) | ||
| } | ||
|
|
||
| // cloudlog(c.get('requestId'), 'signedURL', device_id, version_name, version.name) | ||
| if (version_name === version.name) { | ||
| cloudlog({ requestId: c.get('requestId'), message: 'No new version available', id: device_id, version_name, version: version.name, date: new Date().toISOString() }) | ||
| // TODO: check why this event is send with wrong version_name | ||
| await sendStatsAndDevice(c, device, [{ action: 'noNew', versionName: version.name }]) | ||
| return simpleError200(c, 'no_new_version_available', 'No new version available') | ||
| } | ||
|
|
||
| if (channelData) { | ||
| // cloudlog(c.get('requestId'), 'check disableAutoUpdateToMajor', device_id) | ||
| if (!channelData.channels.ios && platform === 'ios') { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -102,6 +102,39 @@ | |||||||||||||||||||||||||||||||||||||||||
| await resetAppDataStats(APPNAME) | ||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Delta manifest test constants | ||||||||||||||||||||||||||||||||||||||||||
| const DELTA_APPNAME = `com.demo.app.delta.${id}` | ||||||||||||||||||||||||||||||||||||||||||
| let deltaOldVersionId: number | null = null | ||||||||||||||||||||||||||||||||||||||||||
| let deltaNewVersionId: number | null = null | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Helper to insert multiple manifest entries for a version | ||||||||||||||||||||||||||||||||||||||||||
| async function insertMultipleManifestEntries(appVersionId: number, entries: { file_name: string, file_hash: string, s3_path: string }[]) { | ||||||||||||||||||||||||||||||||||||||||||
| const supabase = getSupabaseClient() | ||||||||||||||||||||||||||||||||||||||||||
| // First, delete any existing manifest entries for this version | ||||||||||||||||||||||||||||||||||||||||||
| await supabase.from('manifest').delete().eq('app_version_id', appVersionId) | ||||||||||||||||||||||||||||||||||||||||||
| // Insert all manifest entries in a single batch | ||||||||||||||||||||||||||||||||||||||||||
| const { error } = await supabase.from('manifest').insert( | ||||||||||||||||||||||||||||||||||||||||||
| entries.map(entry => ({ | ||||||||||||||||||||||||||||||||||||||||||
| app_version_id: appVersionId, | ||||||||||||||||||||||||||||||||||||||||||
| file_name: entry.file_name, | ||||||||||||||||||||||||||||||||||||||||||
| s3_path: entry.s3_path, | ||||||||||||||||||||||||||||||||||||||||||
| file_hash: entry.file_hash, | ||||||||||||||||||||||||||||||||||||||||||
| file_size: 100, | ||||||||||||||||||||||||||||||||||||||||||
| })), | ||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||
| if (error) | ||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`Failed to insert manifest entries: ${error.message}`) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Update the manifest_count on the version | ||||||||||||||||||||||||||||||||||||||||||
| await supabase.from('app_versions').update({ manifest_count: entries.length }).eq('id', appVersionId) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // Also update manifest_bundle_count on the app to enable manifest fetching | ||||||||||||||||||||||||||||||||||||||||||
| const { data: version } = await supabase.from('app_versions').select('app_id').eq('id', appVersionId).single() | ||||||||||||||||||||||||||||||||||||||||||
| if (version) { | ||||||||||||||||||||||||||||||||||||||||||
| await supabase.from('apps').update({ manifest_bundle_count: entries.length }).eq('app_id', version.app_id) | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+131
to
+134
|
||||||||||||||||||||||||||||||||||||||||||
| // Also update manifest_bundle_count on the app to enable manifest fetching | |
| const { data: version } = await supabase.from('app_versions').select('app_id').eq('id', appVersionId).single() | |
| if (version) { | |
| await supabase.from('apps').update({ manifest_bundle_count: entries.length }).eq('app_id', version.app_id) | |
| // In tests, set manifest_bundle_count to a non-zero value to enable manifest fetching | |
| const { data: version } = await supabase.from('app_versions').select('app_id').eq('id', appVersionId).single() | |
| if (version) { | |
| await supabase.from('apps').update({ manifest_bundle_count: 1 }).eq('app_id', version.app_id) |
Check failure on line 354 in tests/updates-manifest.test.ts
GitHub Actions / Run tests
tests/updates-manifest.test.ts > delta manifest scenarios > returns full manifest for first install (builtin version)
AssertionError: expected undefined to be defined
❯ tests/updates-manifest.test.ts:354:27
Check failure on line 354 in tests/updates-manifest.test.ts
GitHub Actions / Run tests
tests/updates-manifest.test.ts > delta manifest scenarios > returns full manifest for first install (builtin version)
AssertionError: expected undefined to be defined
❯ tests/updates-manifest.test.ts:354:27
Check failure on line 354 in tests/updates-manifest.test.ts
GitHub Actions / Run tests
tests/updates-manifest.test.ts > delta manifest scenarios > returns full manifest for first install (builtin version)
AssertionError: expected undefined to be defined
❯ tests/updates-manifest.test.ts:354:27
Check failure on line 354 in tests/updates-manifest.test.ts
GitHub Actions / Run tests
tests/updates-manifest.test.ts > delta manifest scenarios > returns full manifest for first install (builtin version)
AssertionError: expected undefined to be defined
❯ tests/updates-manifest.test.ts:354:27
Copilot
AI
Jan 18, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test modifies shared application data (manifest entries for version 1.0.1) but runs sequentially, which is correct. However, the comment on line 378 could be more specific about why this test must run sequentially - it's not just because it modifies data, but because it modifies version data that could be used by other delta manifest tests running concurrently.
| // This test modifies data (inserts manifest entries for version 1.0.1), so it runs sequentially | |
| // This test must run sequentially because it inserts manifest entries for version 1.0.1, | |
| // i.e. shared version data that other delta manifest tests may read; running it concurrently | |
| // could cause those tests to observe modified version state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incomplete cleanup: manifest_bundle_count not reset on app.
The insertMultipleManifestEntries helper updates manifest_bundle_count on the app (line 134), but the cleanup only resets manifest_count on the version. This could leave stale state affecting other tests.
🐛 Fix cleanup to reset app manifest_bundle_count
finally {
// Clean up: remove the manifest entries we just added
if (identicalVersion) {
await supabase.from('manifest').delete().eq('app_version_id', identicalVersion.id)
await supabase.from('app_versions').update({ manifest_count: 0 }).eq('id', identicalVersion.id)
+ // Also reset manifest_bundle_count on the app to match beforeAll state
+ const { data: version } = await supabase.from('app_versions').select('app_id').eq('id', identicalVersion.id).single()
+ if (version) {
+ // Restore to the count from beforeAll (3 entries on deltaNewVersionId)
+ await supabase.from('apps').update({ manifest_bundle_count: 3 }).eq('app_id', version.app_id)
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| finally { | |
| // Clean up: remove the manifest entries we just added | |
| if (identicalVersion) { | |
| await supabase.from('manifest').delete().eq('app_version_id', identicalVersion.id) | |
| await supabase.from('app_versions').update({ manifest_count: 0 }).eq('id', identicalVersion.id) | |
| } | |
| } | |
| finally { | |
| // Clean up: remove the manifest entries we just added | |
| if (identicalVersion) { | |
| await supabase.from('manifest').delete().eq('app_version_id', identicalVersion.id) | |
| await supabase.from('app_versions').update({ manifest_count: 0 }).eq('id', identicalVersion.id) | |
| // Also reset manifest_bundle_count on the app to match beforeAll state | |
| const { data: version } = await supabase.from('app_versions').select('app_id').eq('id', identicalVersion.id).single() | |
| if (version) { | |
| // Restore to the count from beforeAll (3 entries on deltaNewVersionId) | |
| await supabase.from('apps').update({ manifest_bundle_count: 3 }).eq('app_id', version.app_id) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In @tests/updates-manifest.test.ts around lines 412 - 418, The finalizer
currently only resets manifest_count on app_versions but not the app's
manifest_bundle_count updated by insertMultipleManifestEntries; after deleting
the manifest rows and updating app_versions in the finally block, also call
supabase.from('apps').update({ manifest_bundle_count: 0 }).eq('id',
identicalVersion.app_id') (or .eq('id', identicalVersion.appId) if that is the
field) to reset the app's manifest_bundle_count to 0 so no stale state remains
for other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NOT EXISTS subquery filters by app_version_id first, then file_name and file_hash. While the existing composite index (file_name, file_hash, app_version_id) can support this query, PostgreSQL will likely use the idx_manifest_app_version_id index instead for the initial app_version_id filter, then check file_name and file_hash conditions. For optimal performance with the delta manifest query, consider creating an additional index on (app_version_id, file_name, file_hash) if profiling shows this query is a bottleneck. However, this is not critical since the existing indexes should provide reasonable performance.