Repository navigation
test(cortex): pin forget batching and a retry that finds the events gone - #314
Conversation
forget_events sends at most FORGET_BATCH (100) ids per request, and a hosted retry that answers 404 means an earlier send already removed them, which is success. Neither was tested. The new test forgets 250 ids against a backend that fails each batch's first send with 503 and answers its retry 404.
|
Warning Review limit reached
This review includes 1 billable file and costs up to $0.25. Or wait 47 minutes for your next included review. View limit details
Comment |
Tiny Sweeper reviewReviewed head: Verdict: Ready for maintainer review · Priority: none Diagnostics and supporting detailsFull summaryPull request purpose (title): test(cortex): pin forget batching and a retry that finds the events gone. Tiny Sweeper completed its review; deterministic results follow. Review snapshot
Completeness: Complete What changedA single new test, `a_forget_is_sent_in_batches_and_a_retry_that_finds_them_gone_succeeds`, is added to the hosted engine test file. It stands up an axum router on `/memory/forget`, captures each request body, and alternates responses by call count: odd sends get 503, even sends get 404. It then calls `engine.log.forget_events("user:alice/app:learnings", &ids)` with 250 ids and asserts the captured per-request `memory_ids` lengths equal `[100, 100, 100, 100, 50, 50]`. No production paths are modified. The critique lane noted a fragility, not a defect: the `% 2` alternation keys off handler call count rather than batch identity, so the test's expectation holds only while each batch is sent exactly twice — it would misfire if `FORGET_BATCH` or `HOSTED_FORGET_ATTEMPTS` changed. The critique lane also observed the code index is behind this pull request (indexed at `1f4681b009b7`) and retrieved context may be out of date. The latest-push-delta evidence is unavailable; no base-to-head diff comparison was trusted. FeaturesNone identified with supported citations. TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. FindingsNo active actionable findings. Before mergeNone. Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
tinysweeper
Bot
added
the
priority: p3
Summary
Test only. Two pieces of
forget_eventshad no test.FORGET_BATCH(100) event ids per request, so 250 ids go as 100, 100 and 50.404means an earlier send already removed them, and that counts as success. A404on the first send is still an error.The new test runs a hosted engine against a small backend. That backend fails each batch's first send with
503and answers its retry404. Forgetting 250 ids must succeed, and the recorded request bodies must name 100, 100, 100, 100, 50 and 50 ids (each batch sent twice).Related issue
None. Found in an edge-case audit (a coverage gap; the behaviour is correct).
API or behavior changes
None.
Validation
Commands actually run, with their outcome:
cargo fmt --all -- --check. rustfmt--check(edition 2024) passed on the touched file. CI runs the full check.cargo clippy --all-targets --all-features -- -D warnings. Left to CI.cargo build --all-targets --all-features. Left to CI.cargo test -p tinymemory-integrations --lib a_forget_is_sent_in_batches_and_a_retry_that_finds_them_gone_succeeds:FORGET_BATCHset to 125: FAILED,left: [125, 125, 125, 125],right: [100, 100, 100, 100, 50, 50].NotFound("[HTTP_404] …").Tests
New
a_forget_is_sent_in_batches_and_a_retry_that_finds_them_gone_succeedsin the hosted engine tests.Documentation
Not needed (test only).
Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the description