Skip to content

OUT-3804: reconcile early-stamped assemblyFileId on create retry#110

Merged
SandipBajracharya merged 2 commits into
mainfrom
OUT-3804
Jun 1, 2026
Merged

OUT-3804: reconcile early-stamped assemblyFileId on create retry#110
SandipBajracharya merged 2 commits into
mainfrom
OUT-3804

Conversation

@SandipBajracharya

Copy link
Copy Markdown
Collaborator

What

retryCreateInAssembly now reconciles a row's early-stamped assemblyFileId before re-creating, so a sweeper retry can't orphan the existing Assembly file or produce a duplicate.

Branches on the existing Assembly file's state:

State Action
completed mark row synced + bump count, no re-create
pending & < 1 day old wait, retry next sweep
pending & ≥ 1 day old null the id → delete the file → re-create
404 re-create

Refactor

  • Extracted the inline block into reconcileExistingAssemblyFile (returns a "should re-create" boolean).
  • Hoisted the magic timeout to STUCK_PENDING_THRESHOLD_MS.
  • De-duplicated the three identical channelSync lookups into getChannelSync.

Race safety

The stale-reclaim path nulls assemblyFileId before the API delete, so the resulting file.deleted webhook can't match this row and race the re-create. A unit test pins this ordering via invocationCallOrder.

Tests

New resync-failed-files.helper.test.ts covers all reconcile branches (completed / pending-young / pending-stale / 404 / no-id / source-gone). The 404 test throws a properly-shaped CopilotApiError and uses the real guard logic, so it actually exercises the production isCopilotApiError check.

pnpm typecheck, pnpm lint, and the resync + tombstone vitest suites all pass.

Related: OUT-3645

🤖 Generated with Claude Code

retryCreateInAssembly now checks the existing Assembly file before
re-creating, so a retry can't orphan it or produce a duplicate:
completed -> mark synced; pending & young -> wait; pending & stale ->
null id, delete, re-create; 404 -> re-create. Extracted the block into
reconcileExistingAssemblyFile, hoisted STUCK_PENDING_THRESHOLD_MS, and
shared the channelSync lookup. Added unit tests for each branch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@linear-code

linear-code Bot commented Jun 1, 2026

Copy link
Copy Markdown

OUT-3804

@vercel

vercel Bot commented Jun 1, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
dropbox-integration Ready Ready Preview, Comment Jun 1, 2026 11:26am

Request Review

@SandipBajracharya SandipBajracharya changed the title fix(OUT-3804): reconcile early-stamped assemblyFileId on create retry OUT-3804: reconcile early-stamped assemblyFileId on create retry Jun 1, 2026
@greptile-apps

greptile-apps Bot commented Jun 1, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a sweeper retry bug where an early-stamped assemblyFileId could cause duplicate Assembly files or orphaned uploads. It introduces reconcileExistingAssemblyFile to branch on the existing file's state before deciding whether to re-create, and adds getChannelSync to de-duplicate three identical DB lookups.

  • Reconcile logic: four branches — already-completed (mark synced, no re-create), pending-young (wait), pending-stale (null id → delete → re-create), 404 (re-create directly). The null-before-delete ordering prevents a file.deleted webhook from racing the re-create.
  • Race safety: assemblyFileId is nulled in the DB before calling deleteFile, so a concurrent webhook handler can't match the row during deletion. A unit test pins this ordering via invocationCallOrder.
  • Test coverage: new resync-failed-files.helper.test.ts exercises all six reconcile branches including a properly-shaped CopilotApiError for the 404 path.

Confidence Score: 3/5

Safe to merge for the common cases, but the age-gate in reconcileExistingAssemblyFile silently swallows any non-completed, non-pending Assembly file status — worth confirming the API's full status surface before landing.

The reconcile logic only explicitly handles completed and, implicitly, pending. Any other status the Assembly API might return (e.g. failed, error) falls through to the 24-hour age-gate: young files get a misleading 'still pending' retry message and re-creation is delayed by up to a day, while only stale ones are correctly deleted and re-created. If the Assembly API's file status is strictly binary (completed | pending), this is a non-issue; but if other statuses are possible, the retry loop silently misclassifies them.

src/features/workers/resync-failed-files/helper/resync-failed-files.helper.ts — specifically the reconcileExistingAssemblyFile function's post-completed fallthrough.

Important Files Changed

Filename Overview
src/features/workers/resync-failed-files/helper/resync-failed-files.helper.ts Adds reconcileExistingAssemblyFile to handle early-stamped assemblyFileId on create retries; logic gap: non-pending/non-completed statuses fall through to the age-gate and trigger misleading markFailure or spurious delete.
src/features/workers/resync-failed-files/helper/tests/resync-failed-files.helper.test.ts New test file covering all described reconcile branches (completed, pending-young, pending-stale, 404, no-id, source-gone); no test for unexpected/non-pending Assembly file statuses, which is where the logic gap in the helper lives.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[retryCreateInAssembly] --> B{dbxFileId / itemPath / channelSync present?}
    B -- No --> Z[markFailure / markDeleted]
    B -- Yes --> C{Dropbox file exists?}
    C -- No / deleted --> D[markDeleted]
    C -- Yes --> E[reconcileExistingAssemblyFile]

    E --> F{assemblyFileId present?}
    F -- No --> G[return true]
    F -- Yes --> H[retrieveFile]

    H -- completed --> I[markUpdated + updateChannelMapSyncedFilesCount
return false]
    H -- other status --> J{row age < 24h?}
    J -- Yes --> K[markFailure 'still pending'
return false]
    J -- No --> L[updateFileMap assemblyFileId=null
deleteFile
return true]
    H -- 404 --> G

    G --> M[completePendingAssemblyCreate]
Loading

Reviews (1): Last reviewed commit: "fix(OUT-3804): reconcile early-stamped a..." | Re-trigger Greptile

Comment thread src/features/workers/resync-failed-files/helper/resync-failed-files.helper.ts Outdated
})
return true
} catch (error) {
if (!isCopilotApiError(error) || error.status !== 404) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be || or &&?

My understanding is if 404, then it means file does not exist, right? But I think that should go to .catch in line 274.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ||. This basically means if the error is not related to copilotError or the request responded with 404 not found we assume the file is not available and we return true.

Will update this try catch block. Thanks for the suggestion

if (!failedSync.assemblyFileId) return true

try {
const existing = await copilotApi.retrieveFile(failedSync.assemblyFileId)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better to return true early, if existing is not defined, etc.

Suggested change
const existing = await copilotApi.retrieveFile(failedSync.assemblyFileId)
const existing = await copilotApi.retrieveFile(failedSync.assemblyFileId).catch((e: AxiosError) => es.status == 404 ? null : throw e)
if (!existing) return true

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update this try catch block. Thanks for the suggestion.

'retryCreateInAssembly: Assembly file still pending upload, will retry next sweep',
)
return false
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be retried though? If upload is in progress and this request did not intiate it, then retry would be processed by the original request that initiated the upload, wouldn't it?

@SandipBajracharya SandipBajracharya Jun 1, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it should be retried. The original request retry for 3 attempts and if the attempt still fails (due to uploads failure), the status is always on pending. We retry and check if status is pending for more than a day. If so we delete that pending record from assembly and create new one and upload again.

Resolve the early-stamped assemblyFileId 404 case at the retrieveFile
call so a missing file routes to the create path, while non-404 Copilot
errors propagate for retry instead of being swallowed. Removes the now
pass-through try/catch.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@priosshrsth priosshrsth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@SandipBajracharya SandipBajracharya merged commit 07d0c01 into main Jun 1, 2026
4 checks passed
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.

2 participants