Implement burn state reset over a new SDK verb#389
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR completes the ChangesState Reset Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f080d2f74c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let rt = tokio::runtime::Builder::new_current_thread() | ||
| .enable_all() | ||
| .build()?; | ||
| let opts = RawIngestOptions::default(); |
There was a problem hiding this comment.
--ledger-path into reset re-ingest options
run_reset opens the ledger with globals.ledger_path, but the --reingest path calls ingest_all with RawIngestOptions::default(), which leaves ledger_home unset. In that case ingest-side config and pending-stamp resolution fall back to RELAYBURN_HOME/default home, so burn --ledger-path <custom> state reset --force --reingest can re-ingest with the wrong sidecar state (miss pending stamps, wrong content mode) even though it writes into the custom DB. Pass the same home override into ingest options here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b09d1dc. run_reset_reingest now takes the --ledger-path override from globals.ledger_path and threads it into RawIngestOptions::ledger_home, so the reingest sweep resolves config and pending-stamp manifests under the same home as the open handle (no more split-brain between custom DB and default $RELAYBURN_HOME sidecars).
Note: commands/summary.rs::run_ingest has the same shape and is similarly affected. Left it out of this PR since it's a pre-existing bug on a different surface; happy to file a follow-up if you want it tracked.
Generated by Claude Code
| .query_row(&format!("SELECT COUNT(*) FROM {table}"), [], |r| r.get(0)) | ||
| .unwrap_or(0); |
There was a problem hiding this comment.
count_reset_targets() converts query_row failures to 0 via unwrap_or(0), which means schema/corruption/read errors in burn.sqlite are silently reported as successful zero-count dry-runs. Because this method returns Result<ResetSummary>, callers expect DB failures to surface; swallowing them can mislead operators and automation into thinking reset is safe/no-op when the ledger is actually unhealthy.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b09d1dc. count_reset_targets() now uses ? on every query_row (no more unwrap_or(0) swallowing schema/corruption errors), and there's a new SDK test (count_reset_targets_propagates_sql_errors) that drops the turns table out from under an open handle and asserts the call returns Err.
Generated by Claude Code
Replace the "not yet implemented" stub with a real presenter that dry-runs by default, wipes via `--force`, and optionally re-ingests via `--force --reingest`. Adds `Ledger::reset()` and `count_reset_targets()` to the SDK; reset is the harder-hitting sibling of `rebuild_derivable` — it also drops stamps and blanks ingest cursors so the next ingest walks every upstream file from offset 0. Closes #341. https://claude.ai/code/session_01VcLNz8KtnLe7zox9xdvfyj
willwashburn
force-pushed
the
claude/check-issue-341-lkbiJ
branch
from
f080d2f to
b09d1dc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
CHANGELOG.md (1)
9-16: ⚡ Quick winDrop the issue link per changelog guidelines.
The entry content is good (impact-first, names command/API, describes practical effect), but it includes
(#341)at the end. As per coding guidelines, issue/PR links should be dropped from changelog entries.Suggested fix
same invocation. Replaces the prior "not yet implemented" stub. - (`#341`)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` around lines 9 - 16, Remove the issue/PR link suffix "(`#341`)" from the CHANGELOG entry that begins with "`relayburn-cli` / `relayburn-sdk`: `burn state reset`..." so the descriptive text remains unchanged but complies with changelog guidelines; also scan that same CHANGELOG.md entry for any other parenthesized issue/PR references and delete them if present.crates/relayburn-sdk/src/ledger.rs (1)
420-463: 💤 Low valueAtomicity gap between
burn.sqliteandcontent.sqlitewipes.The
burn.sqlitewipe (derivable tables + stamps + archive_state reset) runs in a transaction (lines 427-444), but thecontent.sqlitewipe (lines 449-461) runs separately afterward. If the content wipe fails after the burn transaction commits, the ledger is left in an inconsistent state: cursors are blanked but content rows still exist.This mirrors the existing
rebuild_derivable()behavior (line 305), so it may be intentional given the two-DB architecture, but worth confirming the failure mode is acceptable or if a rollback-on-content-failure strategy is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/relayburn-sdk/src/ledger.rs` around lines 420 - 463, The reset() function currently commits the burn.sqlite transaction (conns.burn) before executing the content.sqlite wipe (conns.content), creating an atomicity gap similar to rebuild_derivable(); to fix, make the two wipes atomic by coordinating them: perform the content wipe (the same batch used now) before committing the burn transaction or use an explicit two-DB failure-recovery pattern (e.g., begin a savepoint/prepare step, run conns.content.execute_batch, and only commit conns.burn after content succeeds, or on content failure roll back the burn transaction), updating reset() to ensure either both burn changes and content changes succeed or the burn changes are rolled back; reference reset(), conns.burn transaction(), the DERIVABLE_TABLES loop, and conns.content.execute_batch when locating where to change the sequence/transactional handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@CHANGELOG.md`:
- Around line 9-16: Remove the issue/PR link suffix "(`#341`)" from the CHANGELOG
entry that begins with "`relayburn-cli` / `relayburn-sdk`: `burn state
reset`..." so the descriptive text remains unchanged but complies with changelog
guidelines; also scan that same CHANGELOG.md entry for any other parenthesized
issue/PR references and delete them if present.
In `@crates/relayburn-sdk/src/ledger.rs`:
- Around line 420-463: The reset() function currently commits the burn.sqlite
transaction (conns.burn) before executing the content.sqlite wipe
(conns.content), creating an atomicity gap similar to rebuild_derivable(); to
fix, make the two wipes atomic by coordinating them: perform the content wipe
(the same batch used now) before committing the burn transaction or use an
explicit two-DB failure-recovery pattern (e.g., begin a savepoint/prepare step,
run conns.content.execute_batch, and only commit conns.burn after content
succeeds, or on content failure roll back the burn transaction), updating
reset() to ensure either both burn changes and content changes succeed or the
burn changes are rolled back; reference reset(), conns.burn transaction(), the
DERIVABLE_TABLES loop, and conns.content.execute_batch when locating where to
change the sequence/transactional handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6709e35e-7c11-4837-9efa-55677870f948
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
CHANGELOG.mdcrates/relayburn-cli/Cargo.tomlcrates/relayburn-cli/src/commands/state.rscrates/relayburn-cli/tests/smoke.rscrates/relayburn-sdk/src/ledger.rscrates/relayburn-sdk/src/ledger/tests.rscrates/relayburn-sdk/src/lib.rs
The 2.4.0 release commit on main bumped each `packages/relayburn` and `packages/sdk-node` optional-dep specifier from 2.3.0 to 2.4.0 in package.json but did not regenerate `pnpm-lock.yaml`. CI's `pnpm install --frozen-lockfile` therefore fails on every PR rebased onto post-2.4.0 main with `ERR_PNPM_OUTDATED_LOCKFILE`. This is a pure specifier sync: no resolved-version churn, no new deps. https://claude.ai/code/session_01VcLNz8KtnLe7zox9xdvfyj
2 participants
Summary
Replaces the
burn state reset"not yet implemented" stub with a realpresenter, addressing the remaining live half of #341. (The
burn summaryhalf —
--by-tool,--quality,--workflow, etc. — has already beenwired up in tree, so this PR closes the last surface stub the issue called
out.)
Ledger::reset()andLedger::count_reset_targets()in
crates/relayburn-sdk/src/ledger.rs.reset()is the harder-hittingsibling of
rebuild_derivable: it wipes every derivable table, thestampsfirst-party table, the entirecontent.sqlite(FTS indexincluded), and resets
archive_state(preservesschema_version,blanks
upstream_cursors_json/last_built_at/last_rebuild_at)so the next ingest walks every upstream file from offset 0.
run_resetincrates/relayburn-cli/src/commands/state.rsnowdispatches three modes:
count_reset_targets(), prints "would dropN event row(s) + M stamp(s) + K content row(s)"
--force→ real wipe viareset()--force --reingest→ wipe then drive a singleingest_allsweepon the same handle (mirrors the runtime pattern in
commands/summary.rs::run_ingest)--jsonoutput paths covered.--jsonreturns{ executed, rowsDropped, stampsDropped, contentRowsDropped, reingest? }.Closes #341.
Test plan
cargo test -p relayburn-sdk --lib reset— three new SDK testscover: full wipe semantics (derivable + stamps + content + cursors +
FTS);
count_reset_targets()is read-only and idempotent;reset()is idempotent on an empty ledger (the
archive_staterow survivesthe bootstrap CHECK constraint).
cargo test -p relayburn-cli --test smoke— three new CLI smoketests cover: dry-run does not mutate (asserts the "dry run" /
--forcebreadcrumbs, and that both DB files exist after);--forceemits the structured JSON envelope with
executed: true;--reingestwithout
--forceis rejected at parse time by clap (the existingrequires = "force"constraint).cargo test --workspace— full suite green (no regressions inthe ledger / ingest / analyze paths).
reset()SQL: derivable + stamps wipesin a single
BEGIN…COMMITso a partial failure can't leave theevents DB half-emptied; FTS sync triggers are dropped/recreated
around the bulk delete (same dance as
rebuild_derivable).https://claude.ai/code/session_01VcLNz8KtnLe7zox9xdvfyj
Generated by Claude Code