fix(e2b): stabilize pagination for duplicate timestamps

[chacha923]

What this PR does

Fixes pagination for GET /v2/sandboxes and snapshot listing when multiple objects share the same timestamp sort key.

The previous cursor only stored the timestamp of the last returned item. Since claim timestamps and checkpoint creation timestamps can collide, the next page searched for the first item strictly after that timestamp and skipped remaining objects with the same timestamp.

This PR adds an optional stable cursor to the internal paginator. When a unique key is configured, pagination now sorts and resumes by (sort key, unique key), encodes the cursor as an opaque base64 JSON token, and keeps raw sort-key tokens compatible for existing clients.

Changes

• Add optional GetUniqueKey support to pkg/utils/pagination.Paginator.
• Use sandbox ID as the tiebreaker for sandbox listing.
• Use Kubernetes object identity (namespace/name) as the tiebreaker for snapshot listing while keeping the response SnapshotID unchanged.
• Add regression coverage for duplicate sandbox claim timestamps, duplicate checkpoint creation timestamps, and duplicate checkpoint IDs across different checkpoint objects.

Tests

go test ./pkg/utils/pagination
go test ./pkg/sandbox-manager -run 'TestSandboxManager_ListSandboxes|TestSandboxManager_ListCheckpoints'
go test ./pkg/servers/e2b -run 'TestListSandboxes_Pagination|TestListSandboxes_PaginationDuplicateClaimTime|TestListSnapshots'
go test ./pkg/utils/pagination ./pkg/sandbox-manager ./pkg/servers/e2b
git diff --check

Fixes #439

[kruise-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign zmberg for approval by writing /assign @zmberg in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kruise-bot]

Welcome @chacha923! It looks like this is your first PR to openkruise/agents 🎉

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.92%. Comparing base (c1cb28c) to head (047a53e).
⚠️ Report is 10 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
+ Coverage   79.67%   79.92%   +0.25%     
==========================================
  Files         194      202       +8     
  Lines       13784    14848    +1064     
==========================================
+ Hits        10983    11868     +885     
- Misses       2403     2551     +148     
- Partials      398      429      +31     

Flag	Coverage Δ
unittests	79.92% <100.00%> (+0.25%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[chacha923]

Hello @furykerry @AiRanthem @zmberg ,
I wanted to politely follow up on this PR. The CI checks are passing, and I believe it is ready for review. I would appreciate any feedback whenever you have the opportunity.
Thank you!

[AiRanthem]

Thanks for working on this. The overall stable-cursor direction looks right to me.

For GET /v2/sandboxes, this should fix the duplicate claim-timestamp pagination skip issue: the paginator now sorts and resumes by (sort key, unique key), the cursor is encoded as an opaque base64url JSON token, and sandbox listing uses sandboxID as the stable tiebreaker. That addresses the core problem where multiple sandboxes share the same timestamp and the next page previously skipped the remaining objects with the same key.

One thing I think should be fixed before merge: snapshot listing currently uses cp.CheckpointID as GetUniqueKey. CheckpointID is not guaranteed to be unique across all list-visible checkpoint objects; there are existing scenarios where the same checkpoint ID can appear in different namespaces, and an admin listing can see both. If two checkpoints have the same CreationTimestamp and the same CheckpointID, the cursor key is still not unique, so the same skip bug can remain for snapshots.

Could we change the snapshot tiebreaker to the Kubernetes object identity instead, for example:

GetUniqueKey: func(cp infra.CheckpointInfo) string {
    return cp.Namespace + "/" + cp.Name
}

or include namespace/name as a suffix if you still want CheckpointID in the token?

It would also be good to add a regression test for snapshots where multiple checkpoints have the same creation timestamp and the same checkpoint ID but different namespace/name, with pagination limit crossing that duplicate-key boundary. The PR description currently says there is a namespace/name fallback, but the current code appears to return only CheckpointID.

After that adjustment, I think this PR should cover the duplicate timestamp pagination issue much more robustly.

[kruise-bot]

@chacha923: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[chacha923]

Thanks for working on this. The overall stable-cursor direction looks right to me.

For GET /v2/sandboxes, this should fix the duplicate claim-timestamp pagination skip issue: the paginator now sorts and resumes by (sort key, unique key), the cursor is encoded as an opaque base64url JSON token, and sandbox listing uses sandboxID as the stable tiebreaker. That addresses the core problem where multiple sandboxes share the same timestamp and the next page previously skipped the remaining objects with the same key.

One thing I think should be fixed before merge: snapshot listing currently uses cp.CheckpointID as GetUniqueKey. CheckpointID is not guaranteed to be unique across all list-visible checkpoint objects; there are existing scenarios where the same checkpoint ID can appear in different namespaces, and an admin listing can see both. If two checkpoints have the same CreationTimestamp and the same CheckpointID, the cursor key is still not unique, so the same skip bug can remain for snapshots.

Could we change the snapshot tiebreaker to the Kubernetes object identity instead, for example:

GetUniqueKey: func(cp infra.CheckpointInfo) string {
    return cp.Namespace + "/" + cp.Name
}

or include namespace/name as a suffix if you still want CheckpointID in the token?

It would also be good to add a regression test for snapshots where multiple checkpoints have the same creation timestamp and the same checkpoint ID but different namespace/name, with pagination limit crossing that duplicate-key boundary. The PR description currently says there is a namespace/name fallback, but the current code appears to return only CheckpointID.

After that adjustment, I think this PR should cover the duplicate timestamp pagination issue much more robustly.

Thanks for the review. I updated the snapshot pagination cursor according to your suggestion.

Snapshot listing now uses the Kubernetes object identity (namespace/name) as the stable tiebreaker instead of CheckpointID, while the response SnapshotID remains unchanged. I also added a regression test covering multiple checkpoints with the same CreationTimestamp and the same CheckpointID but different namespace/name, with pagination crossing that duplicate-key boundary.

@AiRanthem

[AiRanthem]

Fixes #581