Skip to content

fix(lite): make delete-on-empty armings prospective#314

Merged
shikhar merged 4 commits intomainfrom
codex/fix-lite-doe-prospective
Mar 8, 2026
Merged

fix(lite): make delete-on-empty armings prospective#314
shikhar merged 4 commits intomainfrom
codex/fix-lite-doe-prospective

Conversation

@shikhar
Copy link
Member

@shikhar shikhar commented Mar 6, 2026

Closes #274.

Summary

  • evaluate expired delete-on-empty entries using their original (deadline, min_age) pairs instead of mixing maxima
  • keep delete-on-empty armings prospective-only: create and disabled-to-enabled transitions can arm new deadlines, while later DOE or retention reconfigs only affect future armings
  • use one conservative DOE arm-delay calculation across append, reconfigure, and trim paths so finite-retention streams still get revisited without adding DOE state

Testing

  • just fmt
  • cargo clippy -p s2-lite --all-targets -- -D warnings
  • cargo test -p s2-lite stream_doe

Notes

  • just test hit an unrelated local-environment failure in s2-cli::cli missing_access_token because s2 list-basins succeeded on this machine with existing auth

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR fixes a correctness bug in the delete-on-empty (DOE) background task and tightens the semantics of DOE deadline arming. The bug was that PendingStreamDoe collapsed multiple pending (deadline, min_age) entries for a stream into a single pair of maxima, which could prevent a stream from being deleted when a lower-deadline/lower-min_age entry was actually eligible while the aggregated maximum values were not. The fix replaces the aggregated struct with Vec<PendingDoeEntry> and evaluates each entry's original pair independently using any().

Key changes:

  • Bug fix (stream_doe.rs): stream_doe_is_eligible now checks each stored (deadline, min_age) entry independently — any() — instead of using aggregated (max_deadline, max_min_age).
  • Unified delay helper (streamer.rs): doe_arm_delay(retention_age, min_age) = retention_age + min_age + DOE_DEADLINE_REFRESH_PERIOD is now the single formula used across all arming paths (append, trim, create, reconfigure).
  • Prospective-only arming (streams.rs): create_stream and reconfigure_stream only arm a new deadline for the initial None → Some DOE enable transition; changes to an already-enabled DOE config (different min_age, changed retention policy) no longer write a new deadline — the next organic arming (via append or trim) will pick up the updated configuration.
  • Test coverage: Four new integration tests directly exercise the corrected eligibility logic, the enable-transition arming, and the prospective-only invariant.

Confidence Score: 5/5

  • This PR is safe to merge — it is a targeted, well-tested bug fix with no new external dependencies or protocol changes.
  • All three changed files have a single, coherent purpose. The eligibility fix is mathematically straightforward (max-aggregation replaced by per-entry any()), and the prospective-only arming guard is clearly expressed and directly tested by two new integration tests. The shared doe_arm_delay helper eliminates the previously divergent delay calculations without changing observable semantics for existing deployments. No unsafe code, no schema migrations, and existing tests continue to pass.
  • No files require special attention.

Important Files Changed

Filename Overview
lite/src/backend/bgtasks/stream_doe.rs Core DOE logic refactored: replaces PendingStreamDoe (which aggregated maxima) with Vec<PendingDoeEntry>, evaluating each (deadline, min_age) pair independently via any(), fixing incorrect eligibility checks; arm_doe_maybe now uses the unified doe_arm_delay helper; four new tests added covering the fixed bug, enable transition, and prospective-only arming invariant.
lite/src/backend/streamer.rs Extracts doe_arm_delay and retention_age_or_zero as pub(super) helpers; inline deadline arithmetic inside maybe_doe_deadline is replaced with the shared helper, unifying the delay formula across all DOE arming paths.
lite/src/backend/streams.rs Both create_stream and reconfigure_stream adopt the doe_arm_delay helper for the deadline timestamp and tighten the arming guard: arming now fires only on new-stream creation or the None → Some DOE enable transition, suppressing re-arming on min_age / retention-policy changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[DOE Arming Paths] --> B[create_stream new stream]
    A --> C[create_stream CreateOrReconfigure]
    A --> D[reconfigure_stream]
    A --> E[arm_doe_maybe after trim empties stream]
    A --> F[maybe_doe_deadline on append]

    B --> G[arm: doe_arm_delay\nretention_age plus min_age plus 600s]
    C -->|prior DOE was None| G
    C -->|prior DOE was Some| SKIP1[skip - prospective only]
    D -->|prior DOE was None, now Some| G
    D -->|prior DOE already Some| SKIP2[skip - prospective only]
    E --> G
    F -->|retention_age present plus refresh elapsed| G

    G --> H[(KV entry: deadline plus stream_id = min_age)]

    H --> I[tick_stream_doe: list expired entries]
    I --> J{stream_has_records?}
    J -->|yes| K[clear deadlines, skip delete]
    J -->|no| L{any entry satisfies:\nwrite_ts plus min_age le deadline?}
    L -->|yes| M[delete_stream]
    L -->|no| K
    M --> N[clear_doe_deadlines]
    K --> N
Loading

Last reviewed commit: 08714f5

@shikhar shikhar marked this pull request as draft March 6, 2026 22:00
@shikhar shikhar force-pushed the codex/fix-lite-doe-prospective branch from c5cb119 to 02a7abd Compare March 8, 2026 01:26
@shikhar
Copy link
Member Author

shikhar commented Mar 8, 2026

@greptileai rereview

@shikhar shikhar force-pushed the codex/fix-lite-doe-prospective branch from 62edaef to af8fbc2 Compare March 8, 2026 03:21
@shikhar shikhar marked this pull request as ready for review March 8, 2026 03:28
@shikhar
Copy link
Member Author

shikhar commented Mar 8, 2026

@greptileai final review

@shikhar shikhar merged commit 0a521ae into main Mar 8, 2026
17 checks passed
@shikhar shikhar deleted the codex/fix-lite-doe-prospective branch March 8, 2026 03:59
@s2-release-plz s2-release-plz bot mentioned this pull request Mar 8, 2026
shikhar pushed a commit that referenced this pull request Mar 13, 2026
## 🤖 New release

* `s2-api`: 0.27.9 -> 0.27.10 (✓ API compatible changes)
* `s2-lite`: 0.29.21 -> 0.29.22 (✓ API compatible changes)
* `s2-cli`: 0.29.21 -> 0.29.22

<details><summary><i><b>Changelog</b></i></summary><p>

## `s2-api`

<blockquote>

## [0.27.10] - 2026-03-13

### Performance

- Avoid intermediate SSE batch allocations
([#320](#320))

<!-- generated by git-cliff -->
</blockquote>

## `s2-lite`

<blockquote>

## [0.29.22] - 2026-03-13

### Bug Fixes

- Make delete-on-empty armings prospective
([#314](#314))

### Performance

- Avoid intermediate SSE batch allocations
([#320](#320))

<!-- generated by git-cliff -->
</blockquote>

## `s2-cli`

<blockquote>

## [0.29.22] - 2026-03-13

<!-- generated by git-cliff -->
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).

Co-authored-by: s2-release-plz[bot] <262023388+s2-release-plz[bot]@users.noreply.github.com>
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.

[Detail Bug] Delete-on-empty: streams may not be auto-deleted when multiple DOE deadlines exist with different min_age

1 participant