Skip to content

fix: avoid skipping orphan file records - #1634

Closed
efegokdemir wants to merge 1 commit into
apache:mainfrom
efegokdemir:codex/issue-1631-cursor-pagination
Closed

efegokdemir wants to merge 1 commit into
apache:mainfrom
efegokdemir:codex/issue-1631-cursor-pagination

Conversation

@efegokdemir

@efegokdemir efegokdemir commented Sep 24, 2026 •

Copy link
Copy Markdown

Summary

Fixes #1631.

CleanOrphanUploadFiles currently requests page 2 after mutating the page-1 rows from status=available to status=deleted. Because the filtered result set shrinks, offset pagination can skip records.

This change repeatedly reads the first page while cleanup changes the result set, and stops when a pass makes no progress. This preserves the existing behavior for recent or referenced files while ensuring later eligible records are examined.

Changes

  • Re-read the first available page after cleanup changes status or repairs a file record.
  • Stop when no record was changed, preventing repeated scans of retained records.
  • Add a regression test with 1,001 orphan records to cover the page-boundary skip.

Testing

  • go test ./internal/service/file_record ./internal/repo/file_record -count=1
  • go vet ./internal/service/file_record ./internal/repo/file_record
  • git diff --check

The focused regression test fails on the pre-fix implementation because record 1,001 remains available, and passes with this change.

AI assistance

Codex was used for repository analysis, reproduction, implementation, and test development. The submitting contributor is responsible for reviewing and understanding the complete patch and its test results.

Signed-off-by: Efe Gökdemir <efe@rexcode.co.uk>
@LinkinStars

Copy link
Copy Markdown
Member

Thanks for the contribution — re-reading page 1 after the available result set changes does fix the original offset-shift case.

However, there is still a correctness gap: if the first page contains 1,000 records that are all retained (for example, still referenced, created within 48 hours, or active branding/avatar files), changed remains false and the loop exits. Orphan records on page 2 or later are then never examined during that cleanup run.

For example, 1,000 retained available records followed by one older orphan record would leave that orphan untouched. Please add coverage for this scenario if this approach is continued.

PR #1635 addresses both cases with ordered ID-cursor pagination (id > lastID): deleting records no longer shifts the scan position, while retained records do not prevent later records from being visited. It also includes multi-page regression coverage, so it appears to be the more complete fix.

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.

CleanOrphanUploadFiles may skip records when using offset pagination

2 participants