Repository navigation
fix(api): scope attachment.resolve to visible cloud workspaces and reap files with the box - #8712
superset-app[bot] wants to merge 1 commit into
Conversation
…ap files with the box
First API step of the attachment plan (SUPER-2874, seam 6).
- attachment.resolve no longer signs a URL for any composer upload in the
org. A kept (ready) file resolves only through a cloud workspace the
caller can see. An upload still in flight (pending) stays org-wide: a
host resolves with its owner's credential, and the sender can be a
teammate on that host. A box still sees only its own workspace's files.
- New attachment.resolveEach returns one entry per id, with
{ fileId, missing: true } for a file that is gone or not visible, so a
transcript can render the rest. resolve keeps its output and errors for
released hosts.
- The archive's final reap stage (and the fallback when it cannot be
queued) detaches and deletes the files anchored to the workspace.
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
|
Preview update published for development build |
 [Medium impact] The changes affect attachment access and cleanup within cloud workspace workflows. Fix the restore race before merging; an old cleanup job can permanently delete a restored workspace's files. FindingsFix with agent prompt### Issue 1
packages/trpc/src/router/cloud-workspace/reap.ts:40
The job checks the archive only before settling usage and deleting the sandbox. During those waits, `unarchive` can restore the same workspace, and `attachFiles` can add new files. The later `reapWorkspaceFiles` call removes every attachment for that workspace ID, including the new ones, and deletes their bytes.
Coordinate cleanup with `unarchive` and `attachFiles` so an old archive cannot delete a restored workspace's files. The queue-failure fallback has the same problem.
### Issue 2
packages/trpc/src/router/cloud-workspace/reap.ts:50-60
The `.catch` does not cover `detachAll`: its awaited argument runs before `reapOrphanFiles` returns a promise. If that database delete fails, `reapWorkspaceFiles` throws instead of logging the cleanup failure.
In the queue-failure fallback, this makes `cloudWorkspace.delete` fail after the sandbox is already gone. It also skips the remaining activity write and update notice. Wrap both cleanup calls in the same `try`/`catch`.
```suggestion
try {
await reapOrphanFiles(
await detachAll({
parentKind: "cloud_workspace",
parentIds: [cloudWorkspaceId],
}),
);
} catch (error) {
console.error(
`[cloud-workspace] ${cloudWorkspaceId} file reap failed`,
error,
);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryThis PR narrows attachment reads and adds workspace file cleanup.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Attachment IDs] --> B[Check organization and file type]
B --> C{Box caller?}
C -->|Yes| D[Require attachment to its workspace]
C -->|No| E[Require pending upload or visible workspace]
D --> F[Check uploaded bytes and sign URL]
E --> F
G[Final archive cleanup] --> H[Delete sandbox]
H --> I[Detach workspace files]
I --> J[Delete files with no remaining attachments]
Reviews (1) · Last reviewed commit: "fix(api): scope attachment.resolve to vi..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| console.error(`[cloud-workspace] ${row.id} final settle failed`, error), | ||
| ); | ||
| await deleteSandbox(row.providerSandboxId); | ||
| await reapWorkspaceFiles(row.id); |
There was a problem hiding this comment.
The job checks the archive only before settling usage and deleting the sandbox. During those waits, unarchive can restore the same workspace, and attachFiles can add new files. The later reapWorkspaceFiles call removes every attachment for that workspace ID, including the new ones, and deletes their bytes.
Coordinate cleanup with unarchive and attachFiles so an old archive cannot delete a restored workspace's files. The queue-failure fallback has the same problem.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/trpc/src/router/cloud-workspace/reap.ts
Line: 40
Comment:
**Restored workspace files get deleted**
The job checks the archive only before settling usage and deleting the sandbox. During those waits, `unarchive` can restore the same workspace, and `attachFiles` can add new files. The later `reapWorkspaceFiles` call removes every attachment for that workspace ID, including the new ones, and deletes their bytes.
Coordinate cleanup with `unarchive` and `attachFiles` so an old archive cannot delete a restored workspace's files. The queue-failure fallback has the same problem.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| await reapOrphanFiles( | ||
| await detachAll({ | ||
| parentKind: "cloud_workspace", | ||
| parentIds: [cloudWorkspaceId], | ||
| }), | ||
| ).catch((error) => | ||
| console.error( | ||
| `[cloud-workspace] ${cloudWorkspaceId} file reap failed`, | ||
| error, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The .catch does not cover detachAll: its awaited argument runs before reapOrphanFiles returns a promise. If that database delete fails, reapWorkspaceFiles throws instead of logging the cleanup failure.
In the queue-failure fallback, this makes cloudWorkspace.delete fail after the sandbox is already gone. It also skips the remaining activity write and update notice. Wrap both cleanup calls in the same try/catch.
| await reapOrphanFiles( | |
| await detachAll({ | |
| parentKind: "cloud_workspace", | |
| parentIds: [cloudWorkspaceId], | |
| }), | |
| ).catch((error) => | |
| console.error( | |
| `[cloud-workspace] ${cloudWorkspaceId} file reap failed`, | |
| error, | |
| ), | |
| ); | |
| try { | |
| await reapOrphanFiles( | |
| await detachAll({ | |
| parentKind: "cloud_workspace", | |
| parentIds: [cloudWorkspaceId], | |
| }), | |
| ); | |
| } catch (error) { | |
| console.error( | |
| `[cloud-workspace] ${cloudWorkspaceId} file reap failed`, | |
| error, | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/trpc/src/router/cloud-workspace/reap.ts
Line: 50-60
Comment:
**Cleanup errors escape the catch**
The `.catch` does not cover `detachAll`: its awaited argument runs before `reapOrphanFiles` returns a promise. If that database delete fails, `reapWorkspaceFiles` throws instead of logging the cleanup failure.
In the queue-failure fallback, this makes `cloudWorkspace.delete` fail after the sandbox is already gone. It also skips the remaining activity write and update notice. Wrap both cleanup calls in the same `try`/`catch`.
```suggestion
try {
await reapOrphanFiles(
await detachAll({
parentKind: "cloud_workspace",
parentIds: [cloudWorkspaceId],
}),
);
} catch (error) {
console.error(
`[cloud-workspace] ${cloudWorkspaceId} file reap failed`,
error,
);
}
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
What & why
Part of SUPER-2874. This is the first API step of the attachment plan (seam 6, "PR 2"). It is API only, with no client changes.
1.
attachment.resolvehas a tighter scope. Before this change, any org member could get a signed URL for any composer upload in the org, including files sent to a cloud workspace that is private to someone else. The rule is now (router/attachment/resolvable.ts):ready): only through a cloud workspace the caller can see (visibleTo: org-visible, created by the caller, or shared with the caller).pending): org-wide, the same as before. This is different from the plan, which says "the caller uploaded it". A host callsresolvewith its owner's credential, and the person sending can be a teammate on that host (v2_users_hostsmembers). Mobile → teammate's hostimportFromCloudand fix(desktop,host-service): send attachments through storage, not host-service bodies #8704'smaterializeIntoWorkspacewould both break under an uploader-only rule.2. New
attachment.resolveEach. It takes the same input and scope. It returns one entry per id, in order, and a file that is gone or not visible comes back as{ fileId, missing: true }. This is for seam 5 (a transcript renders the files it can and shows a badge for the rest). The plan changesresolve's output, but released host-service builds read.urlfrom every entry, soresolvekeeps its output and itsNOT_FOUND/BAD_REQUESTerrors (trpc-compat rule 5).resolveEachis not inSANDBOX_ALLOWED_PROCEDURES, because a box has no use for it yet.3. Cloud workspace files are reaped with the box. When the archive's final reap stage deletes the sandbox, it now runs
detachAll+reapOrphanFilesfor the files anchored to the workspace. The fallback incloudWorkspace.delete(used when the reap cannot be queued) does the same. Within the 7-day grace period an unarchive keeps its files. If the reap fails, the error is logged, the same as the final settle. Objects whose row delete did not finish are still reclaimed bysweepOrphanFiles.tRPC compatibility
resolve: the input, output, error codes and procedure type are unchanged. Auth is stricter only forreadyfiles. Released callers ofresolve(desktop 1.34–1.39 and cli 1.36–1.39 tags, mobile on main): desktopRichText/uploadFileresolves its ownpendingupload right away; host-serviceimportFromCloud/materializeIntoWorkspaceresolvependinguploads, or run in a box assandboxCaller; mobile cloud chat callsattachFilesand then imports on the box (sandboxCaller). None of them resolves areadyfile as a non-box caller.resolveEach: a new procedure, so the change is additive.Not in this PR
chat_sessionscoping waits for the durable session store.How I tested it
packages/trpc:bun testpasses (675 tests),bun run typecheckpasses, and biome is clean.reap.test.ts: the delete stage reaps the workspace's files. The stop stage, an unarchived workspace and a later archive do not.resolvable.integration.ts(a manual DB test, likesweep.integration.ts): I ran it against this workspace's Neon branch and all 5 tests pass. It covers: a teammate resolves a pending upload; a ready file resolves through an org-visible workspace but not through ajust_meworkspace for a teammate; a ready file anchored to no cloud workspace does not resolve; a box resolves only its own files; a page asset never resolves.Checklist
type(scope): subject)bun run lintandbun run typecheckpass (CI fails on lint warnings too)Summary by cubic
Part of SUPER-2874, the first API step of the attachment plan. API only, no client changes.
Scopes
attachment.resolveso a kept (ready) file resolves only through a cloud workspace the caller can see;pendinguploads stay org-wide because a host resolves with its owner's credential and the sender may be a teammate. A box still sees only its own workspace's files. Addsattachment.resolveEach, which returns one entry per id with{ fileId, missing: true }for files that are gone or not visible.resolvekeeps its output and errors for released hosts.Also reaps cloud workspace files with the box: the archive's final delete stage (and the fallback in
cloudWorkspace.delete) detaches and deletes the files anchored to the workspace; unarchives within the 7-day grace period keep their files, and failed reaps only log the error.Not included: archives on retired providers still keep their files, and
chat_sessionscoping waits for the durable session store.Written for commit 0740a3d. Summary will update on new commits.