Skip to content

fix(rpc): stabilize announce pagination and cap delivery trace state#18

Merged
CoteTommy merged 4 commits intomainfrom
codex/pr17-review-fixes
Feb 13, 2026
Merged

fix(rpc): stabilize announce pagination and cap delivery trace state#18
CoteTommy merged 4 commits intomainfrom
codex/pr17-review-fixes

Conversation

@CoteTommy
Copy link
Collaborator

Summary

This PR addresses both review findings from PR #17 by fixing pagination correctness and unbounded RPC delivery-trace memory usage.

Changes

1) Stable announce pagination cursor

  • Problem: list_announces used only timestamp as the page cursor (next_cursor = timestamp). Records with identical timestamps could be skipped across pages.
  • Fix:
    • Updated cursor format to include a tie-breaker: next_cursor = "<timestamp>:<id>".
    • Updated cursor parser to decode (timestamp, id) pairs.
    • Extended announce listing query path to use stable ordering and predicate:
      • ORDER BY timestamp DESC, id DESC
      • cursor filter: timestamp < ? OR (timestamp = ? AND id < ?)
  • Files changed:
    • crates/reticulum/src/rpc/daemon.rs
    • crates/reticulum/src/storage/messages.rs

2) Cap delivery trace state growth

  • Problem: delivery_traces kept an entry per unique message ID forever; only per-message transitions were capped, causing unbounded growth.
  • Fix:
    • Added overall map cap for tracked message IDs (e.g., 2048 entries).
    • Added eviction logic to remove old IDs when the trace map exceeds the cap.
    • Kept existing per-message transition cap (32 entries) intact.

Validation

  • Commit: 29a9840
  • Branch: codex/pr17-review-fixes

Notes

  • This patch applies to main and keeps existing API shape intact while improving cursor behavior and long-run memory safety.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 29a98401b6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

.map_err(std::io::Error::other)?;
let next_cursor = if items.len() >= limit {
items.last().map(|record| record.timestamp.to_string())
items.last().map(|record| format!("{}:{}", record.timestamp, record.id))

Choose a reason for hiding this comment

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

P1 Badge Make announce cursor boundary match first-page sort order

This cursor now uses timestamp:id, which assumes the page is ordered by timestamp DESC, id DESC, but the initial list_announces query path (before_ts=None) still sorts only by timestamp in MessagesStore::list_announces. When several announces share a timestamp, the first page can return IDs in a different tie order, so the next-page predicate id < last_id may duplicate items already seen or skip unseen ones. Update the no-cursor query to use the same tie-break ordering as the cursor boundary.

Useful? React with 👍 / 👎.

@CoteTommy CoteTommy merged commit 7768f2e into main Feb 13, 2026
2 checks passed
@CoteTommy CoteTommy deleted the codex/pr17-review-fixes branch February 13, 2026 18:54
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.

1 participant