fix(trace_management): delete never terminated above one page - #63
fix(trace_management): delete never terminated above one page#63smiller-comet wants to merge 2 commits into
Conversation
collect_trace_ids() hardcoded page=1 in both of its fetch calls and exited only on a short page. Deletion runs *after* collection completes, so nothing shrinks the result set during the walk and page 1 returns identical rows forever. Any project with more than BATCH_SIZE (1,000) matching traces looped indefinitely, re-adding the same IDs until it ran out of memory — and hammered the API hard enough to exhaust the workspace rate limit, after which the next run reported "0 traces" and exited 0. The README's own example (1,743 matches) is above the threshold. test_manage_traces.py seeds ~70 traces, so it never reached the failing path. Rewritten on the opik SDK's REST client: - Deletion reads a page and deletes it, then repeats. The result set genuinely shrinks, so it terminates; memory is bounded by one page; an interrupted run resumes naturally. A guard aborts if a page is unchanged after a delete. - Non-mutating reads use the API's last_retrieved_id cursor, not offsets. - Reads carry only id + start_time. Trace bodies, aggregates and attachments are excluded server-side rather than transferred and discarded. - --dry-run no longer enumerates: it reports the count, the date window it actually matched, and the batch plan, so previews are fast at any volume. - Rate limits (429) are waited out per the server's Retry-After. - Runtime API errors now set exit 1 instead of printing and reporting success, so a scheduled run can be alerted on. A count that failed prints ERROR rather than 0. Also fixes which date "older than" meant. Date bounds went to to_time, which bounds the trace *id* — i.e. ingestion time — while --after separately filtered start_time, so the two bounds used different clocks. For traces logged as they happen the clocks agree, but for backfilled data they do not: 1,250 traces imported today carrying start_time ~100 days ago are a year old on one clock and seconds old on the other, so which is meant must be explicit rather than incidental. Both bounds now share one clock, defaulting to ingestion — which is what the original --before already used, and the right basis for a retention commitment, since "we do not keep data longer than 90 days" is a claim about custody. It is also the only clock a client cannot influence: start_time is supplied by whatever wrote the trace and accepted as given, so keying deletion on it means a skewed clock or a stamped future date keeps data past its deletion date while the sweep still reports success. --time-field start_time selects the activity-age policy where that is what is meant. The two are never combined; being AND-ed would restore the exclusion. Adds test_pagination.py — offline, no credentials: termination above one page, exactly-once deletion, page boundaries, the no-progress guard, and that date bounds land on exactly one clock, never both. test_manage_traces.py seeds backdated fixtures, so it now forces --time-field start_time; it also warns on exit 1 rather than tolerating it. README documents the two clocks and why the default is custody-based, the OR-within-a-TTL-rule tag semantics, that TTL counts are an upper bound, the exit codes, and a flock+timeout cron recipe. Corrects the stated delete batch size (200, not 1,000). Verified live against a workspace project of 1,860 traces (read-only) and a seeded throwaway project of 1,250 backfilled traces (since removed): list and --dry-run return in ~3s where the previous dry-run never terminated; delete removed 100 by tag then 1,150 across two pages, leaving 0; error paths exit 1; offline suite is 20/20.
Mechanical only — `uv run ruff format .`, no behaviour change. The folder was not format-clean before this branch, and CONTRIBUTING's PR checklist asks for both `ruff check` and `ruff format --check` to pass. Kept as its own commit so the preceding fix reads without reformatting noise.
|
Strong, well-reasoned fix — approving. The delete-as-you-go rewrite is the right shape: the result set genuinely shrinks so the loop terminates, memory is bounded to one page, and interrupted runs resume. Fixing the exit-0-on-failure behavior and the split-clock date bounds are both real correctness wins on their own. Verified against the pinned floor ( Two non-blocking notes for the record:
Minor/optional: Good to merge. |
2 participants
What & why
deletenever terminated on any project with more than 1,000 matching traces — which isessentially every real retention cleanup, including the 1,743-trace example in this
folder's own README.
collect_trace_ids()hardcodedpage=1in both of its fetch calls and exited only on ashort page. Deletion ran after collection finished, so nothing shrank the result set
during the walk and page 1 returned identical rows forever. The loop re-added the same
IDs until it ran out of memory, and hammered the API hard enough to exhaust the
workspace rate limit — after which the next run hit a 429, printed it, reported
Grand total: 0 traces, and exited 0.The loop's own docstring argued that incrementing pages was unsafe because offsets go
stale once you start deleting. That is true — but only for a delete-as-you-go design,
which this wasn't. It inherited the workaround's cost without the condition that
motivated it.
test_manage_traces.pyseeds ~70 traces, so it never reached the failing path.The fix
Rewritten on the
opikSDK's REST client:shrinks, so it terminates; memory is bounded by one page rather than the full match
set; an interrupted run resumes naturally. A guard aborts if a page comes back
unchanged after a delete, instead of spinning.
last_retrieved_idcursor, not offsets.id+start_time. Trace bodies, aggregates and attachments areexcluded server-side rather than transferred and discarded.
strip_attachmentsalsostops the backend downloading attachment blobs for a response we throw away.
--dry-runno longer enumerates. It reports the count, the date window actuallymatched, and the batch plan — so previews are fast at any volume.
Retry-After.be alerted on. A count that failed prints
ERRORrather than0.Which date "older than" means
Date bounds went to
to_time, which bounds the trace id — ingestion time — while--afterseparately filteredstart_time. The two bounds used different clocks.Both now share one clock, defaulting to ingestion: the same basis
--beforealreadyused, and the right one for a retention commitment, since "we don't keep customer data
longer than 90 days" is a claim about custody. It is also the only clock a client cannot
influence —
start_timeis supplied by whatever wrote the trace and accepted as given,so keying deletion on it means a skewed clock or a stamped future date keeps data past
its deletion date while the sweep still reports success.
--time-field start_timeselects the activity-age policy where that is what's meant.The two are never combined; being AND-ed would restore the exclusion.
--dry-runnamesthe clock it used, so a mistaken cutoff is visible before anything is deleted.
Verification
Offline —
test_pagination.py(new; no credentials, no network, milliseconds).20/20: termination above one page, exactly-once deletion, boundaries (0/1/999/1000/1001/4001),
the no-progress guard, and that date bounds land on exactly one clock.
Live, read-only — a 1,860-trace project:
list2.3s,delete --dry-run3.1s. Thesame dry-run on the previous revision ran 45s without terminating and exhausted the
workspace rate limit.
Live, destructive — a seeded throwaway project of 1,250 backfilled traces (deleted
afterwards): 100 removed by tag (1250 → 1150 exactly), then 1,150 removed across two
pages (1000 + 150) → 0. Error paths (unknown project, no filters, unreadable config)
exit 1.
Also caught during the rewrite: the paginated endpoint rejects the
excludefield namesthe streaming endpoint accepts (
400 Invalid query param exclude 'input'), so the twosize=1probes don't send it — they fetch one row, where pruning saves nothing anyway.Notes for the reviewer
have the record.
test_manage_traces.pynow forces--time-field start_time. Its fixtures arecreated now but backdated via
start_time, so on the default clock everydate-based scenario would match nothing while still printing tidy output — it asserts
nothing. It also now warns on exit 1 rather than tolerating it.
run.sh, sopr-test.ymlskips this folder — as it does today, and as it doesfor 4 of the 6
scripts/folders. Worth a separate issue: thelive-runjob executesrun.shwith real credentials, so opting a trace-deletion tool into it needs adeliberately read-only
run.sh, and the DRY_RUN convention fits a tool that requirescredentials poorly.
style: apply ruff format) — the folder was notformat-clean before this branch. Read the fix commit alone to skip that noise.
OPIK_BASE_URLis left as-is rather than renamed to the documentedOPIK_URL_OVERRIDE; changing it would break existing users.retention policy. Deletion is ID-only (
BatchDeleteByProject, capped at 1,000), soevery retention job must enumerate client-side first.
Checklist
scripts)lowercase_with_underscoresREADME.mdhas all required sections; no examples added/renamed/removed, so index tables unchangeduv run ruff check .anduv run ruff format --check .are clean.envfiles committedpyproject.toml(uv project); norequirements.txt, no committeduv.lockrun.sh/ dry-run CI job — not applicable, see notes aboveOPIK_EXAMPLES_MODEL— not applicable, this script makes no LLM calls