Repository navigation
feat(host-service): sweep the attachment store by last use, and keep fileId on cloud copies - #8846
superset-app[bot] wants to merge 1 commit into
Conversation
…fileId on cloud copies Part of the attachments plan (PR 3: seams 8, 2, and the fileId carry-through of seam 3). The host's attachment store had no retention. Nothing deletes chat.db rows and a terminal launch has no session, so there is no session delete to hook. Instead, metadata.json records lastUsedAt. Upload and import set it, and every path that hands an attachment out sets it again: read, agents.run's attached files block, and the ACP resolver. A daily sweep (not in sandboxes) removes entries unused for ATTACHMENT_RETENTION_DAYS. An entry written before this change is stamped, not removed. The check and the rename that takes the entry out of the store run in the same tick, so a hand-out on this event loop cannot land between them. The slow delete runs async, off the loop. A box's cached copy now keeps the fileId it was imported from, in its metadata and in a small .by-file-id index, so the adapters (next PR) can find a cloud part's cached copy without a scan. The first prompt of a cloud launch spreads that metadata into its parts. Without fileId, the first attachments of a cloud session would be the one kind a reader without the box cannot render. MAX_INLINE_ATTACHMENT_BYTES moves to @superset/shared/attachment-limits with MAX_THUMBNAIL_BYTES and ATTACHMENT_RETENTION_DAYS. The host and the shared launch schema use MAX_ATTACHMENTS for their max(10).
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
 [Medium impact] The changes affect attachment storage and chat launches across local hosts and cloud workspaces. Fix the chat launch timing gap before merging so cleanup cannot delete an attachment already accepted for a prompt. FindingsFix with agent prompt### Issue 1
packages/host-service/src/trpc/router/agents/agents.ts:669-671
`chatLaunchTarget` accepts attachments without refreshing `lastUsedAt`. The agent reads them only after its async startup finishes. If an attachment is past retention and the sweep runs during startup, the sweep deletes it before the agent reads it. The prompt then goes through without that attachment.
Use `handOutAttachment` when accepting attachments for a chat launch, as the terminal launch already does.
```suggestion
const attachments: ChatLaunchAttachment[] = [];
for (const attachmentId of input.attachmentIds ?? []) {
const resolved = handOutAttachment(attachmentId);
```
### Issue 2
packages/host-service/src/trpc/router/attachments/sweep.ts:35-38
If a leftover `.sweeping-*` directory cannot be removed, the error stops the entire sweep. Every later entry waits until the next daily pass, which can fail at the same directory again. This can leave expired attachments accumulating elsewhere in the store.
Catch and log failures per entry, then continue cleaning the remaining entries.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds 30-day attachment cleanup, tracks last use, and carries cloud
DiagramsequenceDiagram
participant Launch as Chat launch
participant Store as Attachment store
participant Agent as Agent startup
participant Sweep as Daily sweep
Launch->>Store: Resolve attachment without updating last use
Launch->>Agent: Queue first prompt
Note over Agent: Async startup
Sweep->>Store: Remove expired attachment
Agent->>Store: Resolve queued attachment
Store-->>Agent: Not found
Note over Agent: Send prompt without attachment
Reviews (1) · Last reviewed commit: "feat(host-service): sweep the attachment..." · Reviewed by Greptile |
|
Preview update published for development build |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const attachments: ChatLaunchAttachment[] = []; | ||
| for (const attachmentId of input.attachmentIds ?? []) { | ||
| const resolved = resolveAttachmentPath(attachmentId); |
There was a problem hiding this comment.
chatLaunchTarget accepts attachments without refreshing lastUsedAt. The agent reads them only after its async startup finishes. If an attachment is past retention and the sweep runs during startup, the sweep deletes it before the agent reads it. The prompt then goes through without that attachment.
Use handOutAttachment when accepting attachments for a chat launch, as the terminal launch already does.
| const attachments: ChatLaunchAttachment[] = []; | |
| for (const attachmentId of input.attachmentIds ?? []) { | |
| const resolved = resolveAttachmentPath(attachmentId); | |
| const attachments: ChatLaunchAttachment[] = []; | |
| for (const attachmentId of input.attachmentIds ?? []) { | |
| const resolved = handOutAttachment(attachmentId); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/agents/agents.ts
Line: 669-671
Comment:
**Chat can lose accepted attachments**
`chatLaunchTarget` accepts attachments without refreshing `lastUsedAt`. The agent reads them only after its async startup finishes. If an attachment is past retention and the sweep runs during startup, the sweep deletes it before the agent reads it. The prompt then goes through without that attachment.
Use `handOutAttachment` when accepting attachments for a chat launch, as the terminal launch already does.
```suggestion
const attachments: ChatLaunchAttachment[] = [];
for (const attachmentId of input.attachmentIds ?? []) {
const resolved = handOutAttachment(attachmentId);
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| for (const entry of entries) { | ||
| if (entry.startsWith(SWEEPING_PREFIX)) { | ||
| await rm(join(root, entry), { recursive: true, force: true }); | ||
| continue; |
There was a problem hiding this comment.
If a leftover .sweeping-* directory cannot be removed, the error stops the entire sweep. Every later entry waits until the next daily pass, which can fail at the same directory again. This can leave expired attachments accumulating elsewhere in the store.
Catch and log failures per entry, then continue cleaning the remaining entries.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/trpc/router/attachments/sweep.ts
Line: 35-38
Comment:
**One failure stops all cleanup**
If a leftover `.sweeping-*` directory cannot be removed, the error stops the entire sweep. Every later entry waits until the next daily pass, which can fail at the same directory again. This can leave expired attachments accumulating elsewhere in the store.
Catch and log failures per entry, then continue cleaning the remaining entries.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
Folded into #9064, which now carries the whole attachments plan (commit list in its description). Closing in favor of that PR. |
Summary
PR 3 of the attachments plan (v6): host-service + shared. It covers seam 8 (the limits module), seam 2 (retention of the host store by last use), and the fileId carry-through of seam 3. It does not depend on #8704 (which now includes the API change from #8712). This PR uses only
attachment.resolve, which is on main.Why
attachments.upload/importFromCloudwrite into$HOST_MANIFEST_DIR/attachments/<id>/, and nothing removes those files. There is no session delete to hook: chat-v3 only hascloseSession, nothing deletes chat.db rows, and a terminal launch has no session.attachmentId. That is the one kind of attachment that a reader without the box (web, a shared view, a resumed session on a recreated box) cannot render.MAX_INLINE_ATTACHMENT_BYTESwas a host-local constant, and the per-send10was hard-coded.How it works
lastUsedAtinmetadata.json. Upload and import set it. Every path that gives a file to a client or an agent goes through the newhandOutAttachment, which sets it again:attachments.read, the# Attached filesblock ofagents.run, and the ACP resolver inchat-v3/acpHarnesses.ts.sweepAttachmentStore(attachments/sweep.ts) runs once at startup and then daily, beside the project purge. It does not run in a sandbox: a box's store is only a cache that dies with the box. It removes entries whoselastUsedAtis older thanATTACHMENT_RETENTION_DAYS(30). An entry written before this change has no stamp. The sweep stamps it, so it gets the full 30 days. For each entry, the metadata check and arenameSyncto.sweeping-<id>run in the same tick, so a hand-out on this event loop cannot slip between them. The recursive delete then runs async (fs/promises.rm), off the main loop. A leftover.sweeping-*from a crash is removed on the next pass.importCloudAttachmentswritesfileIdinto the metadata and into a one-line index file.by-file-id/<fileId>→ attachmentId.handOutAttachmentByFileIdreads it in O(1), checks it against the metadata, and stamps the entry. Delete and the sweep remove the index entry with the store entry.fileId.chatLaunchTargetspreadsfileId(andsizeBytes) from the store metadata into each part. The sandbox self-seed already sends the imported ids through it, so a cloud launch's first prompt now names its storage object.importFromCloud's result also gainsfileId(an additive output field).userContentSchema's attachment part gains optionalfileId(uuid) andsizeBytes. These are additive.attachmentIdis still required. Making it optional is the next PR, together with the resolver that can serve a part without it.@superset/shared/attachment-limitsgainsMAX_INLINE_ATTACHMENT_BYTES(moved from the host'sconstants.ts, which is deleted; the attachments index still re-exports it),MAX_THUMBNAIL_BYTES, andATTACHMENT_RETENTION_DAYS.importFromCloud,materializeIntoWorkspaceandcloudAgentLaunch.attachmentFileIdsuseMAX_ATTACHMENTS(still 10, so their inputs are unchanged).Where this differs from the plan, or the plan is out of date against main
attachmentIdandfileId. The plan says "exactly one of the two is set". On main today, every adapter and every released mobile build readsattachmentId, so a fileId-only part would reach the agent as "could not be read" until the resolver from the next PR ships. Readers still branch onfileIdfirst. The next PR can dropattachmentIdfrom host-built parts once its resolver is in place.ATTACHMENT_PARTS_MIN_HOST_VERSIONis not here. It is "the first host release carrying seam 4", so it belongs in the next PR.MAX_THUMBNAIL_BYTESis defined but not used yet. Desktop'suseChatImageSourcekeeps its local copy until the desktop PR switches it over. That keeps this PR to host-service + shared..max(10)inpackages/trpc(cloudWorkspace.create'sattachmentFileIds,attachFiles). They are API code, outside this PR.agents.ts:737is nowsessions/create-chat-session.ts(createChatSession), which spreadschatLaunchTarget's attachments into the prompt.tRPC compatibility
attachments.importFromCloud: the output gainsfileId, which is additive. The input is unchanged (max(MAX_ATTACHMENTS)=max(10)).attachments.materializeIntoWorkspace: the input is unchanged, for the same reason.attachments.read,upload,delete: the contracts are unchanged.readnow also writeslastUsedAt.looseObject.MIN_HOST_SERVICE_VERSIONchange.Testing
sweep.test.ts: the sweep removes an entry unused past retention and keeps a recent one; an unstamped entry is stamped, not removed; an entry handed out after the pass started is kept; the fileId index is removed with its entry;handOutAttachment/handOutAttachmentByFileIdstamp what they hand out.attachments.test.ts:readstamps the entry;importCloudAttachmentsrecords the fileId, and a lookup by that fileId finds the copy (with a localBun.servestanding in for the presigned GET).agents.test.ts: a cloud launch's first-prompt part carries the fileId it was imported from (import, thenchatLaunchTarget); theagents.runattached files block (buildTerminalAgentLaunch) stamps what it lists.bun run typecheckpasses inpackages/shared,packages/chat,packages/chat-runtime,packages/host-serviceandapps/desktop. Biome is clean on the changed files.packages/host-servicebun test: 2644 pass, 1 fail. The failure ishealth.check > serialises the current boot's stamps … in sandbox mode. It reads the real boot stamps of the machine it runs on, and this workspace is a real cloud box, soreadyis true andbundleis set. It does not touch attachment code.no-main-loop-blocking.test.tspasses, and thermSynccount instorage.tsis still 2.packages/shared(1444) andpackages/chat(94): all pass.apps/mobiletypecheck has 190 errors both with and without this change (checked on a clean tree). None of them are in attachment code.Known limitations
metadata.json(a download that failed before its metadata was written) is skipped by the sweep, not removed.