Skip to content

Protect operation attempt iteration (#858) - #860

Merged
AcoPiper merged 2 commits into
mainfrom
AcoPiper/issue-858
Aug 18, 2026
Merged

Protect operation attempt iteration (#858)#860
AcoPiper merged 2 commits into
mainfrom
AcoPiper/issue-858

Conversation

@AcoPiper

@AcoPiper AcoPiper commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

The operation_attempt column family holds records below a reserved byte and secondary index entries above it. Table<OperationAttempt>::iter already bounds its scan below that byte, but the blanket Iterable implementation on Table admitted the table anyway through FromKeyValue: a caller could select the unbounded trait iter with UFCS, or call prefix_iter with an empty or deliberately reserved prefix, and reach index keys that are not records and do not decode as one.

This adds a private iteration::Eligible marker in src/tables.rs, implemented for every current FromKeyValue record type except OperationAttempt, and requires it on the Table blanket Iterable implementation. The marker is an opt-in list rather than a negative bound, so the exclusion holds unless someone removes it on purpose rather than lapsing the next time a positive bound elsewhere is broadened. Nothing is exported, so no new public surface appears and nothing outside the crate can implement it.

The IndexedTable blanket implementation deliberately does not carry the bound — no table that reserves part of its key space is indexed, and NodeTable's hand-written Iterable implementation iterates through that route. Public rustdoc on Iterable, on the operation_attempt module, on OperationAttempt, and on the inherent Table<OperationAttempt>::iter now explains the record/index key-space boundary and why generic iteration excludes this table.

The generic write API remains unavailable for OperationAttempt, so every ledger write still maintains its indexes atomically. Index encoding, single-flight, expiry, sweep, retention, and the persisted record shape are unchanged, and no wall clock is read in the sweep or prune paths.

Per the issue's out-of-scope section, no CHANGELOG.md entry is added: the column family is absent from MAP_NAMES and the table has no Store accessor, so this change is not reachable from the last release.

Closes #858

Test plan

  • cargo fmt -- --check --config group_imports=StdExternalCrate passes
  • cargo clippy --bins --tests --all-features -- -D warnings passes
  • cargo test --all-features passes
  • The paired rustdoc regression on OperationAttempt holds: the plain doctest taking &Table<TorExitNode> compiles and the compile_fail,E0599 doctest taking &Table<OperationAttempt> does not, with both making the identical table.prefix_iter(todo!(), None, b"") call, importing Iterable, and leaving the iterator and the Direction argument inferred
  • The existing compile_fail regressions for the generic write API on OperationAttempt still fail to compile
  • The inherent Table<OperationAttempt>::iter test covers a non-terminal attempt with cleanup_state = Some(..), which owns all three index entries, and the bounded scan yields the record with no decoding error
  • Generic iteration is exercised explicitly for Table<CoreComponent>, Table<Agent>, and Table<ExternalService>, through both iter and an empty-prefix prefix_iter
  • NodeTable's hand-written Iterable implementation, including its IndexedTable<InnerNode>-backed route, still compiles and composes each Node from its three column families
  • The iteration::Eligible marker remains unexported and covers every current FromKeyValue record type except OperationAttempt

The `operation_attempt` column family holds records and three prefixed
secondary-index key spaces. Its inherent `iter` bounds the scan below
the reserved range, but the record still implements `FromKeyValue`,
which admitted its table to the blanket `Iterable` implementation: UFCS
selects the unbounded trait `iter`, and `prefix_iter` with an empty or
deliberately reserved prefix reaches index entries, which are not
records and yield a decoding error rather than a row.

Gate that implementation on a private opt-in marker carried by every
other `FromKeyValue` record, so the exclusion holds unless it is removed
on purpose rather than lapsing the next time a positive bound is
broadened. `IndexedTable` keeps its unbounded implementation, leaving
`NodeTable`'s hand-written iteration route unchanged.

Closes #858.
Part of #831.
`NodeTable` carries an inherent `iter` alongside its `Iterable`
implementation, so method syntax in the new iteration test resolved to
the inherent one and left half the test naming a route it did not take.
Only `prefix_iter`, which has no inherent counterpart, reached the trait.

Part of #858.
@codecov

codecov Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.26%. Comparing base (6feedf3) to head (e470fad).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #860      +/-   ##
==========================================
+ Coverage   84.14%   84.26%   +0.11%     
==========================================
  Files          92       92              
  Lines       36216    36331     +115     
==========================================
+ Hits        30474    30613     +139     
+ Misses       5742     5718      -24     

☔ 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.

@AcoPiper

Copy link
Copy Markdown
Contributor Author

[Reviewer Round 1]

No findings.

The private iteration::Eligible list in src/tables.rs:567-607 covers every current FromKeyValue record type except OperationAttempt, and the bound at src/tables.rs:791-796 removes both blanket routes (Iterable::iter via UFCS and prefix_iter) without restricting IndexedTable. The ledger’s inherent iterator remains bounded below the reserved index range at src/tables/operation_attempt.rs:559-573.

The paired rustdocs at src/tables/operation_attempt.rs:227-246 prove the intended public API distinction without depending on construction or concrete iterator types. The expanded iterator test includes an attempt owning all three index entries (src/tables/operation_attempt.rs:1416-1438), while the node regression explicitly invokes the hand-written trait iter through UFCS and checks both its iteration routes (src/tables/node.rs:1342-1362). The PR body correctly links Closes #858 and includes a test plan; the review thread contains no author-created issue.

@AcoPiper

Copy link
Copy Markdown
Contributor Author

[Review Verdict Round 1: APPROVED]

@AcoPiper

Copy link
Copy Markdown
Contributor Author

Suggested squash commit

Title

Exclude the operation ledger from generic scans

Body

The `operation_attempt` column family holds records below a reserved
byte and secondary index entries above it. The inherent
`Table<OperationAttempt>::iter` already bounds its scan below that
byte, but the blanket `Iterable` implementation on `Table` admitted
the table anyway through `FromKeyValue`: a caller could select the
unbounded trait `iter` with UFCS, or call `prefix_iter` with an empty
or deliberately reserved prefix, and reach index keys that are not
records and do not decode as one.

Add a private `iteration::Eligible` marker, implement it for every
current `FromKeyValue` record type except `OperationAttempt`, and
require it on the `Table` blanket implementation. An opt-in list
rather than a negative bound, so the exclusion holds unless someone
removes it on purpose rather than lapsing the next time a positive
bound elsewhere is broadened. Nothing is exported, so no new public
surface appears and nothing outside the crate can implement it.

The `IndexedTable` blanket implementation deliberately does not carry
the bound: no table that reserves part of its key space is indexed,
and `NodeTable`'s hand-written `Iterable` implementation iterates
through that route. Public rustdoc on `Iterable`, on the
`operation_attempt` module, on `OperationAttempt`, and on the inherent
iterator now explains the record/index key-space boundary and why
generic iteration excludes this table.

The generic write API remains unavailable for `OperationAttempt`, so
every ledger write still maintains its indexes atomically. Index
encoding, single-flight, expiry, sweep, retention, and the persisted
record shape are unchanged, and no wall clock is read in the sweep or
prune paths.

No `CHANGELOG.md` entry: the column family is absent from `MAP_NAMES`
and the table has no `Store` accessor, so this change is not reachable
from the last release.

Closes #858

@AcoPiper
AcoPiper merged commit b81c117 into main Aug 18, 2026
10 checks passed
@AcoPiper
AcoPiper deleted the AcoPiper/issue-858 branch August 18, 2026 10:33
@sehkone sehkone mentioned this pull request Aug 22, 2026
6 tasks
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.

Protect operation attempt iteration

1 participant