Skip to content

fix(presigned-url): keep the database claim across request-lane statements - #1616

Merged
pyramation merged 1 commit into
mainfrom
fix/presigned-upload-database-claim
Aug 2, 2026
Merged

fix(presigned-url): keep the database claim across request-lane statements#1616
pyramation merged 1 commit into
mainfrom
fix/presigned-upload-database-claim

Conversation

@pyramation

Copy link
Copy Markdown
Contributor

Summary

uploadAppFile (and the file downloadUrl resolver) failed over HTTP with DATABASE_CLAIM_REQUIRED even though the request carried jwt.claims.database_id.

Root cause: the presigned-url plugin ran multi-statement request work as

withPgClient(pgSettings, (client) => client.query(...) /* then another query */)

The @dataplan/pg adaptor applies pgSettings as transaction-LOCAL set_config(key, value, is_local => true) but does not open an explicit transaction around the callback. So each client.query(...) runs in its own implicit (autocommit) transaction, and the LOCAL claim set for one statement is already gone by the next — jwt_private.current_database_id() then raises DATABASE_CLAIM_REQUIRED. (Confirmed at runtime: the callback's first statement saw the claim, the next did not.)

Fix — a small helper that mirrors the existing pg-query-context pattern (new src/request-pg-client.ts):

export function withRequestPgClient(withPgClient, pgSettings, cb) {
  return withPgClient(null, (client) =>          // system lane: no settings on acquire
    client.withTransaction(async (tx) => {        // ONE explicit transaction
      await applyRequestSettings(tx, pgSettings);  // set_config(..., is_local) INSIDE it
      return cb(tx);                               // every statement sees the claim
    }),
  );
}

All request-lane reads/writes now go through it (plugin.ts, download-url-field.ts): resolveDatabaseId, bucket/file reads, single + bulk processSingleFile, and the delete-middleware pre-read/refcount reads. System-lane config reads (loadAllStorageModules) stay on withPgClient(null, ...).

One extra case: provisionAndRecordPhysicalBucket's physical_name UPDATE is intentionally a system-lane write (privileged role, so it bypasses the RLS that blocks request roles). But the buckets table's catalog-sync trigger calls current_database_id(), so a bare system-lane write hit the same claim error. It now runs through withRequestPgClient(withPgClient, { 'jwt.claims.database_id': databaseId }, ...) — the claim is applied inside the transaction without switching off the privileged role, so RLS stays bypassed and the trigger resolves.

Verification

  • Package build (tsc typecheck) + lint clean.
  • New __tests__/request-pg-client.test.ts (no live DB; models the adaptor's transaction-LOCAL semantics): reproduces the bug (claim lost across autocommit statements), proves the fix (claim durable across statements in the transaction), asserts the system-lane acquire + BEGIN → set_config → work → COMMIT ordering, no set_config when there are no settings, and error propagation + ROLLBACK (no swallowing).
  • End-to-end against a local fun up --k8s cluster (graphql-public rebuilt with this dist/): uploadAppFile returns a presigned URL, PUT200, and a second upload of identical content returns deduplicated: true with uploadUrl: null and the same fileId.

Link to Devin session: https://app.devin.ai/sessions/2ffbeace70364c6aa00b2534a5eadd06
Requested by: @pyramation

…the database claim survives

Presigned upload/download resolvers ran multi-statement request work under withPgClient(pgSettings, cb), which applies jwt claims as transaction-LOCAL set_config without opening an explicit transaction. Each statement ran in its own autocommit transaction, so jwt.claims.database_id was gone by the next statement and jwt_private.current_database_id() raised DATABASE_CLAIM_REQUIRED on uploadAppFile.

Add withRequestPgClient: acquire the client in the system lane (null pgSettings), open one transaction, apply the request settings inside it, then run the callback. Route resolveDatabaseId, bucket/file reads, single/bulk uploads, and delete-middleware reads through it. The system-lane physical_name record-write now carries the tenant database_id claim too, since the buckets catalog-sync trigger calls current_database_id().
@pyramation pyramation self-assigned this Aug 2, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@pyramation
pyramation merged commit a24b9b8 into main Aug 2, 2026
16 checks passed
@pyramation
pyramation deleted the fix/presigned-upload-database-claim branch August 2, 2026 20:04
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.

1 participant