kvcache-ai/AgentENV

Fork 0

mirror of https://github.com/kvcache-ai/AgentENV.git synced 2026-11-05 01:17:36 +00:00

[GH-ISSUE #353] bug(logs): Log pagination cursor is inclusive in both directions, duplicating the boundary entry on every page, with no unique entry IDs to dedupe #89

New issue

Closed

opened 2026-10-29 14:19:26 +00:00 by bsakaguchi · 2 comments

bsakaguchi commented

2026-10-29 14:19:26 +00:00

Owner

Originally created by @tm-worker on GitHub (Oct 10, 2026).
Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/353

Originally assigned to: @zlzGithub-0801 on GitHub.

Affected component

Sandbox logs API pagination (GET /v2/sandboxes/{sandboxID}/logs) — src/sandbox/logs.rs, src/api/openapi.yml

AgentENV version

main @ 34cdc80 ("feat: add sandbox logs API")

Host environment

OS: macOS 15 (darwin/arm64), Rust stable — the pagination logic is platform-independent
Evidence method: the real src/sandbox/logs.rs (git-archive copy of main @ 34cdc80) is compiled via #[path] into a minimal harness that mirrors only crate::logging::LogLevel (verbatim from src/logging.rs), so the exercised code path is the actual file. The same test appended to the file's own test module also passes.

Operation and configuration

Default configuration; any client that pages through logs using the cursor parameter. The response (SandboxLogsV2Response, src/api/openapi.yml:413-422) carries no next-token field, so clients must use the last entry's timestamp as the next page's cursor — the natural E2B SDK pattern.

Steps to reproduce

Simulate the natural client paging loop against the real log_query_window + LogPage::push/finish:

// 10 entries, 10 ms apart (all inside the 7-day window); limit = 4; direction = backward
let mut cursor: Option<i64> = None;
loop {
    let (start, end) = log_query_window(cursor, true);          // backward window (cursor-7d, cursor]
    let mut page = LogPage::new(start, end, limit, true, None, None);
    for e in &entries { page.push(e.clone()); }
    let finished = page.finish();                               // sort desc + truncate, no cursor emitted
    if finished.is_empty() { break; }
    pages.push(stamps_of(&finished));
    cursor = Some(finished.last().unwrap().timestamp.timestamp_millis());  // <- only way to continue
}

Observed pages (ms timestamps, last digits shown for clarity):

page 1 -> [...108, ...098, ...088, ...078]
page 2 -> [...078, ...068, ...058, ...048]
page 3 -> [...048, ...038, ...028, ...018]
page 4 -> [...018]

Page 1's last entry (…078) is page 2's first entry; page 2's last (…048) is page 3's first — every page boundary entry is served on two consecutive pages, in both directions (forward is symmetric).

Expected behavior

Either:

boundary handling is exclusive on the continuation side (backward paging uses ts < cursor), so consecutive pages never repeat an entry; or
the response provides an explicit next cursor/token so clients don't derive one from a millisecond-truncated timestamp; and/or
entries carry unique IDs so clients can dedupe reliably.

The chosen semantics should be documented in openapi.yml (the cursor description currently does not mention inclusivity).

Actual behavior

src/sandbox/logs.rs:86-99: push() keeps entries with timestamp < self.start || timestamp > self.end — both bounds inclusive; every page boundary entry is served twice across pages in both directions.
SandboxLogsV2Response (src/api/openapi.yml:413-422) contains only logs — no nextToken/nextCursor field; log entries have no unique IDs.
Client-side dedupe is impossible in general: identical adjacent lines (a repeated error message) are intentionally preserved as separate entries within a page (comment at src/sandbox/logs.rs:109), so a client cannot distinguish "cursor duplicated this entry" from "the guest really logged this twice".
The current inclusive behavior is locked in by an existing test (src/sandbox/logs.rs:303-313, sandbox_logs_cursor_is_inclusive_and_handles_int64_max) — so this may be intentional; if so, the cross-page duplication and the lack of a dedupe mechanism should at least be documented.

Logs and diagnostics

running 5 tests
test paging_demo::backward_paging_loop_demonstrates_boundary_duplication ... ok
test sandbox_logs::tests::sandbox_logs_backward_paging_across_two_pages ... ok
test sandbox_logs::tests::sandbox_logs_cursor_is_inclusive_and_handles_int64_max ... ok
test sandbox_logs::tests::sandbox_logs_merge_filters_before_limit_and_preserves_equal_timestamps ... ok
test sandbox_logs::tests::sandbox_logs_decoder_handles_svlogd_chunking_and_flat_fields ... ok

test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out

page 1 -> [1791615029108, 1791615029098, 1791615029088, 1791615029078]
page 2 -> [1791615029078, 1791615029068, 1791615029058, 1791615029048]
page 3 -> [1791615029048, 1791615029038, 1791615029028, 1791615029018]
page 4 -> [1791615029018]

(One note from the same run: when the dataset spans far less than the 7-day window — as in the test data — the oldest entry repeats until the client's "short page" heuristic stops the loop, since the inclusive window never drops it. With logs spanning the full window, termination happens once cursor - 7d passes the oldest entries.)

Reproduction frequency

Always, for any query that returns a full page and is paged further (both directions).

Regression information

Not a regression — the pagination semantics shipped with the new logs API (34cdc80).

Additional context

Fix sketch: make the continuation bound exclusive (backward: ts < cursor; forward: ts > cursor) and update logs.rs:303-313 accordingly, or add an explicit opaque next_cursor to SandboxLogsV2Response. Note timestamps are millisecond-truncated, so exclusivity alone still collapses entries sharing the same millisecond — worth deciding explicitly (e.g. tie-break by file/sequence in the cursor). I'm happy to open a PR with the fix and the accompanying test.

Pre-submission checklist

I searched existing open and closed issues and did not find a duplicate.
I provided a minimal reproduction (the paging loop above, driven through the real src/sandbox/logs.rs).
I removed credentials, tokens, private image references, and other sensitive information.
This is not a security vulnerability.

Originally created by @tm-worker on GitHub (Oct 10, 2026). Original GitHub issue: https://github.com/kvcache-ai/AgentENV/issues/353 Originally assigned to: @zlzGithub-0801 on GitHub. ### Affected component Sandbox logs API pagination (`GET /v2/sandboxes/{sandboxID}/logs`) — `src/sandbox/logs.rs`, `src/api/openapi.yml` ### AgentENV version `main` @ `34cdc80` ("feat: add sandbox logs API") ### Host environment - OS: macOS 15 (darwin/arm64), Rust stable — the pagination logic is platform-independent - Evidence method: the **real** `src/sandbox/logs.rs` (git-archive copy of `main` @ `34cdc80`) is compiled via `#[path]` into a minimal harness that mirrors only `crate::logging::LogLevel` (verbatim from `src/logging.rs`), so the exercised code path is the actual file. The same test appended to the file's own `test` module also passes. ### Operation and configuration Default configuration; any client that pages through logs using the `cursor` parameter. The response (`SandboxLogsV2Response`, `src/api/openapi.yml:413-422`) carries **no next-token field**, so clients must use the last entry's timestamp as the next page's cursor — the natural E2B SDK pattern. ### Steps to reproduce Simulate the natural client paging loop against the real `log_query_window` + `LogPage::push`/`finish`: ```rust // 10 entries, 10 ms apart (all inside the 7-day window); limit = 4; direction = backward let mut cursor: Option<i64> = None; loop { let (start, end) = log_query_window(cursor, true); // backward window (cursor-7d, cursor] let mut page = LogPage::new(start, end, limit, true, None, None); for e in &entries { page.push(e.clone()); } let finished = page.finish(); // sort desc + truncate, no cursor emitted if finished.is_empty() { break; } pages.push(stamps_of(&finished)); cursor = Some(finished.last().unwrap().timestamp.timestamp_millis()); // <- only way to continue } ``` Observed pages (ms timestamps, last digits shown for clarity): ```text page 1 -> [...108, ...098, ...088, ...078] page 2 -> [...078, ...068, ...058, ...048] page 3 -> [...048, ...038, ...028, ...018] page 4 -> [...018] ``` Page 1's **last** entry (`…078`) is page 2's **first** entry; page 2's last (`…048`) is page 3's first — every page boundary entry is served on two consecutive pages, in both directions (forward is symmetric). ### Expected behavior Either: - boundary handling is exclusive on the continuation side (backward paging uses `ts < cursor`), so consecutive pages never repeat an entry; or - the response provides an explicit next cursor/token so clients don't derive one from a millisecond-truncated timestamp; and/or - entries carry unique IDs so clients can dedupe reliably. The chosen semantics should be documented in `openapi.yml` (the cursor description currently does not mention inclusivity). ### Actual behavior - `src/sandbox/logs.rs:86-99`: `push()` keeps entries with `timestamp < self.start || timestamp > self.end` — **both bounds inclusive**; every page boundary entry is served twice across pages in both directions. - `SandboxLogsV2Response` (`src/api/openapi.yml:413-422`) contains only `logs` — no `nextToken`/`nextCursor` field; log entries have no unique IDs. - Client-side dedupe is impossible in general: identical adjacent lines (a repeated error message) are intentionally preserved as separate entries within a page (comment at `src/sandbox/logs.rs:109`), so a client cannot distinguish "cursor duplicated this entry" from "the guest really logged this twice". - The current inclusive behavior is locked in by an existing test (`src/sandbox/logs.rs:303-313`, `sandbox_logs_cursor_is_inclusive_and_handles_int64_max`) — so this may be intentional; if so, the cross-page duplication and the lack of a dedupe mechanism should at least be documented. ### Logs and diagnostics ```text running 5 tests test paging_demo::backward_paging_loop_demonstrates_boundary_duplication ... ok test sandbox_logs::tests::sandbox_logs_backward_paging_across_two_pages ... ok test sandbox_logs::tests::sandbox_logs_cursor_is_inclusive_and_handles_int64_max ... ok test sandbox_logs::tests::sandbox_logs_merge_filters_before_limit_and_preserves_equal_timestamps ... ok test sandbox_logs::tests::sandbox_logs_decoder_handles_svlogd_chunking_and_flat_fields ... ok test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out page 1 -> [1791615029108, 1791615029098, 1791615029088, 1791615029078] page 2 -> [1791615029078, 1791615029068, 1791615029058, 1791615029048] page 3 -> [1791615029048, 1791615029038, 1791615029028, 1791615029018] page 4 -> [1791615029018] ``` (One note from the same run: when the dataset spans far less than the 7-day window — as in the test data — the oldest entry repeats until the client's "short page" heuristic stops the loop, since the inclusive window never drops it. With logs spanning the full window, termination happens once `cursor - 7d` passes the oldest entries.) ### Reproduction frequency Always, for any query that returns a full page and is paged further (both directions). ### Regression information Not a regression — the pagination semantics shipped with the new logs API (`34cdc80`). ### Additional context Fix sketch: make the continuation bound exclusive (backward: `ts < cursor`; forward: `ts > cursor`) and update `logs.rs:303-313` accordingly, or add an explicit opaque `next_cursor` to `SandboxLogsV2Response`. Note timestamps are millisecond-truncated, so exclusivity alone still collapses entries sharing the same millisecond — worth deciding explicitly (e.g. tie-break by file/sequence in the cursor). I'm happy to open a PR with the fix and the accompanying test. ### Pre-submission checklist - [x] I searched existing open and closed issues and did not find a duplicate. - [x] I provided a minimal reproduction (the paging loop above, driven through the real `src/sandbox/logs.rs`). - [x] I removed credentials, tokens, private image references, and other sensitive information. - [x] This is not a security vulnerability.

bsakaguchi closed this issue

2026-10-29 14:19:26 +00:00

bsakaguchi commented

2026-10-29 14:19:27 +00:00

Author

Owner

@zlzGithub-0801 commented on GitHub (Oct 14, 2026):

Thanks for reporting this. We’ll keep the current behavior to maintain compatibility with the E2B SDK’s semantics.

@zlzGithub-0801 commented on GitHub (Oct 14, 2026): Thanks for reporting this. We’ll keep the current behavior to maintain compatibility with the E2B SDK’s semantics.

bsakaguchi commented

2026-10-29 14:19:27 +00:00

Author

Owner

@tm-worker commented on GitHub (Oct 14, 2026):

Thanks for the clarification. Would you mind documenting the inclusive cursor
semantics in the OpenAPI cursor description for the logs endpoint? Clients
paging with the last entry's timestamp currently have no way to know that the
boundary entry repeats on the next page.

@tm-worker commented on GitHub (Oct 14, 2026): Thanks for the clarification. Would you mind documenting the inclusive cursor semantics in the OpenAPI `cursor` description for the logs endpoint? Clients paging with the last entry's timestamp currently have no way to know that the boundary entry repeats on the next page.

bsakaguchi referenced this issue

2026-10-29 14:20:37 +00:00

[PR #89] [MERGED] feat(cli): add shell completion scaffolding #172

bsakaguchi referenced this issue

2026-10-29 14:20:50 +00:00

[PR #115] [CLOSED] feat(cli): install shell completion loaders #192