Skip to content

test(drive): retry transient async cleanup contention - #2397

Open
yxzhaao wants to merge 3 commits into
larksuite:mainfrom
yxzhaao:fix/stabilize-slides-screenshot-cleanup
Open

test(drive): retry transient async cleanup contention#2397
yxzhaao wants to merge 3 commits into
larksuite:mainfrom
yxzhaao:fix/stabilize-slides-screenshot-cleanup

Conversation

@yxzhaao

@yxzhaao yxzhaao commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Stabilize Drive-backed E2E cleanup when concurrent deletes under the same folder make an asynchronous delete task report server_error: drive task failed.

Changes

  • Centralize the targeted retry in drive.DeleteDriveResourceAndVerify so Slides, Docs, Sheets, Base, Wiki, and Drive cleanups share one policy.
  • Verify the resource's real terminal state after each failed delete; retry only when it still exists.
  • Retry only the exact api/server_error/drive task failed envelope, with five attempts and 2s/4s/8s/8s bounded backoff.
  • Keep unrelated permission, validation, authentication, and protocol failures fatal.
  • Simplify the Slides screenshot cleanup back to a direct shared-helper call.

Impact

CLI behavior is unchanged. Live E2E teardown becomes more resilient to the backend's same-folder delete lock while preserving bounded failure behavior.

Root Cause

Live E2E jobs and Go packages run concurrently against the same test bot. Drive serializes deletes within a folder, so overlapping cleanup tasks can collide and one async task may terminate with drive task failed.

Test Plan

  • gofmt
  • git diff --check
  • go vet ./tests/cli_e2e/drive ./tests/cli_e2e/slides
  • Ran TestSlidesScreenshotAliasesLiveE2E in Docker with the fixed E2E app credentials: presentation creation and shared-helper cleanup succeeded; screenshot execution is blocked because that app lacks slides:presentation:screenshot
  • Full live retry path in CI

Related Issues

Summary by CodeRabbit

  • Bug Fixes
    • Improved reliability when deleting Drive resources by retrying transient backend failures with capped exponential backoff.
    • Preserved immediate handling for successful, warning, non-transient, exhausted, and canceled operations.
    • Standardized cleanup behavior in Slides workflows to provide more consistent resource deletion verification.

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 419b810d-86ec-4f84-b354-59afab9ded17

📥 Commits

Reviewing files that changed from the base of the PR and between 2829ecd and 8489a3b.

📒 Files selected for processing (3)
  • tests/cli_e2e/drive/drive_delete_workflow_test.go
  • tests/cli_e2e/drive/helpers.go
  • tests/cli_e2e/slides/slides_screenshot_workflow_test.go
💤 Files with no reviewable changes (1)
  • tests/cli_e2e/drive/drive_delete_workflow_test.go

Included review availability: Your plan includes up to 10 reviews per rolling hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Drive deletion cleanup now retries recognized transient backend failures with capped exponential backoff. Slide screenshot cleanup delegates drive deletion and verification to the shared helper.

Changes

Drive cleanup

Layer / File(s) Summary
Transient drive deletion retry handling
tests/cli_e2e/drive/helpers.go, tests/cli_e2e/drive/drive_delete_workflow_test.go
DeleteDriveResourceAndVerify retries recognized transient drive task failures up to five times with capped exponential backoff. It handles non-transient failures, warnings, exhausted retries, and cancellation without further retries. Unused failure-classification code was removed from the workflow test.
Slide screenshot cleanup integration
tests/cli_e2e/slides/slides_screenshot_workflow_test.go
Slide screenshot cleanup now uses DeleteDriveResourceAndVerify instead of manually executing the drive deletion command.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 8489a

The change adds bounded retries for a narrowly defined transient cleanup failure while preserving fatal handling for unrelated errors. No actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant SlideScreenshotWorkflow
  participant DeleteDriveResourceAndVerify
  participant CLI
  participant DriveAPI
  SlideScreenshotWorkflow->>DeleteDriveResourceAndVerify: delete and verify drive resource
  DeleteDriveResourceAndVerify->>CLI: execute drive deletion
  CLI->>DriveAPI: request drive deletion
  DriveAPI-->>CLI: deletion result
  CLI-->>DeleteDriveResourceAndVerify: command output
  DeleteDriveResourceAndVerify->>CLI: retry transient failure with capped backoff
  DeleteDriveResourceAndVerify-->>SlideScreenshotWorkflow: cleanup result
Loading

Suggested reviewers: wittam-01

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: retrying transient Drive cleanup contention during end-to-end tests.
Description check ✅ Passed The description includes the required summary, changes, test plan, and related issues sections with clear scope and verification details.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the size/S Low-risk docs, CI, test, or chore only changes label Aug 18, 2026
@yxzhaao
yxzhaao marked this pull request as ready for review August 18, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Low-risk docs, CI, test, or chore only changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant