Repository navigation
fix(cleanup): keep shared chat files, sweep inline images by prefix, and make room refetches wait - #9433
Merged
Conversation
The workspace invalidation room awaits a callback's promise before it starts the next delivery, but the type allowed any return, and the tables and workflows rooms fired their invalidations without returning them. A broadcast arriving while a list refetch was still loading invalidated it again, which cancels and restarts the in-flight fetch, so under a burst of writes a slow list could keep restarting. The callback type is now `() => Promise<unknown>`, both rooms return `Promise.all` over the same invalidations they ran before, and the resource-folder picker returns a resolved promise when it has no workspace. Autopsy: the generic hook's tests drive it with hand-written callbacks that return a pending promise, so they proved the hook waits, but nothing exercised a real binding: the tables and workflows rooms were never mounted in a test, and `() => unknown` let a void-returning callback type-check. The new tables room test mounts the real binding against a real QueryClient and fails on the old binding with a second fetch started while the first was pending.
`loadDeployedWorkflowState` asks Postgres to withhold the version blob for the version id it hinted for the workflow, but the hint was written on every materialize miss, by-id reads included, and never on a cache hit. An execution or webhook loading an older version by id repointed the hint away from the active one, and a rollback to a version still in the cache left it on the version rolled back from, so each later active read pulled the full blob again until the hint expired. The hint is now written only by the active read, on every successful return, hit or miss, and `materializeDeploymentState` no longer touches it. Autopsy: the cache tests only ever read the active version, one version per workflow or one cutover, so the hint always matched; no test interleaved a by-id read or returned to a cached version, and the integration test checks answers, which stay correct when the hint is stale (it costs bytes, not correctness). The two new cases queue the withheld row a correct hint produces and fail on the old code with NoActiveDeploymentError.
`collectChatFiles` accepted any truthy `fileAttachments[].key` and then called string methods on it, so one message row carrying a numeric or object key threw and failed the whole chat purge, leaving every other key of the batch behind. It now uses the same guard as the reference check in `chat-cleanup.ts` (`isRecordLike` plus a non-empty string key) and skips the entry. Autopsy: the purge tests only fed well-formed attachments; message content is untyped jsonb written by several clients, and the cast to string hid the gap from type-check. The new case fails on the old code with `key.startsWith is not a function`.
…ript Inline chat images live at `chat-images/<chatId>/<requestId>/<sha>.webp` with no `workspace_files` row, and chat cleanup found them by re-deriving keys from the transcript it read before the delete. An image a turn published after that read, which the admin removal hook and the account deletion pre-read both leave a window for, was never collected and stayed in storage. The `copilot.chat.backend-cleanup` handler now deletes every object under `chat-images/<chatId>/` for each chat it confirms deleted, through the same paged listing and deadline-aware step the Project prefix cleanup uses (moved into `lib/uploads/core/sweep-prefix.ts` and shared), checkpointing how many chats it finished and continuing in a later run near its deadline. `collectChatFiles` no longer derives inline image keys and reads only each message's `fileAttachments`. So that every path still reaches the sweep, chat ids are queued even without `COPILOT_API_KEY`, with a `copilotBackend` flag saying whether they also go to the backend (absent on already-queued events, which keeps their meaning), and the retention jobs behind `prepareChatCleanup` queue an image-only event for the chats they confirm deleted. Events already queued with inline image file entries still delete those keys normally. Comments on the account deletion and admin removal paths now state the residual precisely: chats created after the read, and attachments added to an already-read chat. Autopsy: the tests seeded each chat's images together with the transcript that names them, so collecting keys from the transcript always matched what was stored; none wrote an object after the cleanup read, which is the window the two removal paths leave open. The new integration case writes images after the admin hook's read and fails on the old handler with the late images still on disk; a bystander chat's images stay untouched.
…ences
A fork names its source's storage key wherever it did not copy the file: an
organization attachment, a message-only copilot attachment, or an upload whose
blob copy failed (`publishedFileRefMaps` leaves those keys in the fork's
messages). The fork comment said cleanup deletes a shared key only once no
chat references it, but the reference check covered `assistant/<org>/` keys
alone, so deleting the source chat deleted the upload a fork still showed.
Every collected key now carries the scope whose chats may still reference it
(`organizationId`, or `workspaceId` from its `workspace_files` row, else the
chat's), and `deletableChatFiles` runs the existing batched `@>` containment
over that scope's messages for every key before the handler or the retention
path deletes it. A chat with no workspace or organization records
`workspaceId: null`: no fork can be made from it. The payload field is
optional, so events already queued still parse; for those, the scope is
recovered from the key where it names one (`assistant/<org>/`,
`workspace/<id>/`), a chat's own inline images need no check, and any other
key is kept and logged rather than risk deleting a file a fork shows. The
fork's `for('key share')` on the source row still orders the two: the check
runs only after the source row is gone, and the fork lives in the same scope.
Autopsy: the only shared-key test used an organization attachment, the one
kind the check covered; no test forked a workspace chat whose upload copy
failed, so the unchecked mothership and copilot keys went unnoticed. The new
integration case deletes a source chat whose upload a fork still references,
and fails on the old handler with the object gone; a second case pins how
events queued before this change are handled.
Moves the prefix sweep step into lib/uploads/core/prefix-sweep.ts with the listing and delete helpers it drives, and deletes sweep-prefix.ts. The provider paging cases (a short or empty non-final page, a stalled listing) now test the step itself against a mocked S3 listing, since a same-module call to listStorageKeyPage cannot be intercepted.
…the throw loadDeployedWorkflowState throws first when the active version has no state and Postgres did not withhold one, then picks the cached re-read or the materialization in one expression; behaviour is unchanged.
…ox event
Inline image cleanup gets CHAT_INLINE_IMAGE_CLEANUP_EVENT ({ chatIds }) and
its own handler: it sweeps the chat-images/<chatId>/ prefix of each chat
whose row is gone, ten chats at a time, checkpoints how many leading chats
are finished (the lowest unfinished one), and continues or defers past its
deadline. enqueueChatBackendCleanup and prepareChatCleanup each queue it for
the chats they cover; the backend event drops its copilotBackend flag and
imagesSwept checkpoint and keeps accepting the shipped { chatIds, files }
payload. Completed image events are pruned with the backend ones.
The reference check behind deletableChatFiles now runs one query per scope
that returns only the matched keys (passed as one JSON parameter), after an
EXISTS that skips a scope with no chat left. ChatFileRef carries one
optional scope (absent on legacy events, null when no other chat can
reference the key); the event schema still maps a legacy organizationId to
it. Legacy key-scope recovery reuses extractWorkspaceIdFromStorageKey, and
inlineChatImagePrefix validates the chat id for both the key and the sweep.
Drops tests the integration suites already cover, moves the non-string attachment key case into the PostgreSQL collect test, and makes the image checkpoint test finish chats after the slow one first so it proves the lowest-unfinished checkpoint. The tables room test reads its key from tableKeys.list. inlineChatImagePrefix returns null for an id outside the key grammar, which the event schema admits, so the image sweep skips and logs such an id instead of failing the event on every retry.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 27 files
This PR changes concurrency-sensitive code such as locks, queues, or retries. Ultrareviews find 2.4x more serious bugs than standard reviews. Comment @cubic-dev-ai ultrareview to run one.
Turn on auto-fix | Re-trigger cubic
Contributor
 [Critical impact] The PR appears safe to merge; no new blocking issues were found. SummaryThis PR keeps shared chat files, sweeps inline images by chat prefix, waits for room refetches, and keeps the deployed-state hint tied to the active version.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…rences The reference-check test never seeded or deleted the chat whose files were being cleaned, so it could not tell a key kept by another chat from one kept by the purged chat itself. It now collects the files of a real source chat, deletes that row, and expects only the key a surviving same-scope fork still attaches to be kept.
… stalls The inline image handler continued without consuming an attempt whenever any chat hit the deadline, even when the lowest unfinished chat had stalled and the checkpoint could not advance, so the event could rerun forever. It now continues only after the checkpoint advanced or when the lowest unfinished chat is itself still advancing, and defers otherwise.
…s it A workspace upload kept because a fork attaches it loses its workspace_files row when the source chat is deleted, and collectChatFiles ignored message-level workspace/ keys, so deleting the last fork left the bytes behind. It now also collects an attached workspace/<ws>/ key that no workspace file, legacy file or file version row names, live or soft-deleted, checked in one query per chunk and scoped to the key's workspace, so the reference check still keeps it while another chat there attaches it.
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 27 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
collectChatFiles placed a linked file row in the chat's organization whenever the chat had one, ignoring the workspace the row itself belongs to, so the reference check would look at the wrong scope's chats. A chat holds a workspace or an organization, never both, but a file row's own workspace is its real owner, so it now comes first. Autopsy: the reference-check tests seeded only organization chats with organization keys, so no test placed a workspace-owned row attached in an organization chat.
Collaborator
Author
Collaborator
Author
|
@cubic-dev-ai review this PR |
Contributor
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 27 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes from the v0.9.37 release review that apply to #9419 and #9427.
RoomChangedCallbackis now() => Promise<unknown>, so a callback that drops its invalidation no longer type-checks; the tables and workflows rooms return the invalidations they already made, so a burst no longer cancels and restarts their in-flight list refetchesloadDeployedWorkflowStatesets the hint, to the active version id, on every successful read; by-id reads of other versions no longer repoint it, so active reads stop re-fetching the state blob after an older version executeschat-images/<chatId>/for every chat whose row is gone, paged, concurrent and resumable under the outbox deadline, on every delete path (hard delete, account and admin removal, retention jobs)organizationIdrefs and inline-image keys), with the legacy paths marked for removal once they drainsweepStoragePrefixmoves intolib/uploads/core/prefix-sweep.tsbeside the listing it pagesType of Change
Testing
bun run lint,bun run check:audits(61),bun run type-check,check:test-typesChecklist
test-auditauthoring gate)