Skip to content

fix: prune deleted rollouts from terminal completion log - #547

Open
elren (elrensmin) wants to merge 1 commit into
microsoft:mainfrom
elrensmin:fix/terminal-order-delete-prune
Open

fix: prune deleted rollouts from terminal completion log#547
elren (elrensmin) wants to merge 1 commit into
microsoft:mainfrom
elrensmin:fix/terminal-order-delete-prune

Conversation

@elrensmin

Copy link
Copy Markdown

Description

delete_rollout (agentlightning/server/routes/rollouts.py:216) pops the rollout from _rollouts and _events but never removes its id from _terminal_order. The append-only completion log backing cursor pagination.

Consequences:

  • GET /api/rollouts/terminal reports total_terminal/next_after for rollouts that no longer exist.
  • Re-enqueueing a deleted client-assigned rollout_id and driving it terminal again appends a duplicate entry.
  • Breaks the documented invariant next_after == total_terminal when caught up (asserted at tests/server/test_endpoints.py:395).

Fix (root cause, not symptom)

In delete_rollout, also prune the completion log:

if rollout_id in _terminal_order:
    _terminal_order.remove(rollout_id)

Removing from the list shifts subsequent indices, which is exactly what keeps the cursor consistent. This also fixes the re-enqueue case: after delete prunes the id, re-completion appends exactly once.

Tests

Added two regression tests in tests/server/test_endpoints.py:

  • test_delete_removes_completed_rollout_from_terminal_log: delete a completed rollout, then total_terminal/next_after drop to 1 and only the surviving id is returned.
  • test_delete_and_reenqueue_completion_is_logged_once: delete, re-enqueue with the same id, re-complete → logged exactly once.

Verification

  • uv run pytest → 51 passed, 3 skipped (no regressions; new tests included).
  • uv run ruff check agentlightning/server/routes/rollouts.py tests/server/test_endpoints.py → only 2 pre-existing E501 (line-too-long) at test_endpoints.py:363-364 in the untouched _terminal helper; my additions are clean.
  • uv run ruff format --check on the two changed files → my additions are format-clean; the only remaining diff is pre-existing style drift in the _terminal helper and cursor test.

Note on pre-existing repo-wide failures (out of scope, not introduced here)

The repo does not currently pass its own lint/format gates cleanly. These are pre-existing and unrelated to this change; I deliberately left them untouched to keep the diff minimal:

  • uv run ruff check . → 601 errors across the repo, dominated by W293 (whitespace, 411), E501 (line-too-long, 99), I001 (import sorting, 20), F541 (f-string without placeholders, 17), F401 (unused imports, 16), plus E402, F841, E001, W291, W292, F100, etc.
  • uv run ruff format --check . → 51 files would be reformatted (44 already clean).
  • uv run pyright → not installed in the dev toolchain (pyright is not a dev dependency in pyproject.toml); uvx pyright only reports reportMissingImports for fastapi/pydantic/httpx because of its isolated env, not real type errors.
    There is no CONTRIBUTING doc, no .pre-commit-config.yaml, and no CI lint/test workflow in this repo (only docs + skills deploy workflows), so these are the only gates available.

Copilot AI lite review requested due to automatic review settings August 20, 2026 13:17
@elrensmin

Copy link
Copy Markdown
Author

@microsoft-github-policy-service agree

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes cursor-pagination correctness for the “terminal rollouts” endpoint by ensuring that deleting a rollout also removes its ID from the in-memory completion log used to paginate terminal rollouts.

Changes:

  • Update delete_rollout to prune deleted rollout IDs from _terminal_order.
  • Add regression tests covering deletion effects on /api/rollouts/terminal totals/cursors and the delete→re-enqueue→complete flow.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
agentlightning/server/routes/rollouts.py Prunes deleted rollout IDs from the terminal completion log to keep cursor pagination consistent.
tests/server/test_endpoints.py Adds regression tests to prevent reintroduction of stale/duplicate terminal-log entries after delete and re-enqueue.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +221 to +222
if rollout_id in _terminal_order:
_terminal_order.remove(rollout_id)
@elrensmin
elren (elrensmin) force-pushed the fix/terminal-order-delete-prune branch from e3ad925 to 0e47a67 Compare August 20, 2026 13:41
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.

2 participants