Skip to content

fix(consolidation): a dry run must not advance the dedup census window - #204

Merged
acidkill merged 1 commit into
acidkill:mainfrom
RobertSigmundsson:fix/dry-run-must-not-advance-dedup-cursor
Sep 7, 2026
Merged

acidkill merged 1 commit into
acidkill:mainfrom
RobertSigmundsson:fix/dry-run-must-not-advance-dedup-cursor

Conversation

@RobertSigmundsson

Copy link
Copy Markdown
Contributor

Summary

  • smem consolidate --dry-run no longer writes to the brain: the dedup census advances its window cursor only when the run is real.
  • Adds two tests — one that a dry run leaves the stored cursor exactly where it found it, and a positive control that a real run still rotates it.

Why

_dedup caps the anchors it compares and rotates the window between runs by keeping a cursor in Brain.metadata. That rotation is a good idea, and it is the reason a brain that has outgrown the cap still gets a full census over several passes instead of comparing the same prefix forever. The cursor was advanced unconditionally, outside the if not dry_run guard that protects everything else in the pass, so a dry run persisted the next window before returning. The following real run then started one window later, and the slice the dry run had merely looked at was skipped rather than compared. The blind spot grew in proportion to how often anyone inspected the brain, which is an unkind way to treat the cautious. The cursor is the one piece of state this census keeps between runs, and moving it is a write; --dry-run undertakes not to make any.

Changes

  • engine/consolidation.py: _advance_dedup_cursor is called only when not dry_run. Nothing else moves. dedup_window_start is still reported, so a dry run's summary still says which window it inspected, and a real run rotates exactly as before.
  • tests/unit/test_dedup_alias_edges.py: two tests over a truncated census. The dry-run test asserts the stored cursor is unchanged and that save_brain was never called; the real-run test asserts the cursor still advances by the window size. The second one is a positive control — it passes both before and after the fix, which is how you can tell the guard removed the write and not the rotation.

Test plan

  • pytest tests/unit/test_dedup_alias_edges.py — 48 passed, against 46 on main; the delta is exactly the two tests added here.
  • test_dry_run_does_not_advance_the_window fails on main with AssertionError: a dry run wrote the cursor back. It fails again if engine/consolidation.py alone is reverted whilst the new tests are kept, so it measures this hunk rather than itself.
  • pytest tests/ -m "not stress" -n 4 — 7285 passed, 48 skipped, 1 xfailed. Two tests in tests/unit/test_dashboard_brains_scope.py fail here and fail identically, by name, on main (7283 passed): they want a live SurrealDB and collide with one another under -n. Both pass when that file is run on its own.
  • ruff check src/ tests/ clean; ruff format --check src/ tests/ reports 739 files already formatted.
  • mypy src/ --ignore-missing-imports — success, no issues found in 354 source files.
  • Coverage under the CI gate: 72.41%, against 72.36% on main.
  • CHANGELOG.md untouched — left to the release entry, as with fix(storage): bind datetimes in time comparisons so they select by value #191–fix(memory): refresh content-derived fields on compress, restore, and refine #193.

Related issues

None filed. It is a one-line guard, so a PR seemed kinder than a ticket.

Verified by

@RobertSigmundsson

_dedup rotates its anchor window between runs by persisting a cursor in
Brain.metadata, and it advanced that cursor unconditionally — outside the
`if not dry_run` guard that protects everything else in the pass. So
`smem consolidate --dry-run` wrote to the brain after all: the next real
run started one window later, and the slice the dry run had merely looked
at was skipped rather than compared.

The cursor is the one piece of state the census keeps between runs, and
moving it is a write. It now moves only when the run is real.

Two tests pin it: a dry run over a truncated census leaves the stored
cursor where it found it (fails on main with "a dry run wrote the cursor
back"), and a real run still rotates it, so the guard removes the write
and not the rotation.

@acidkill acidkill left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff. Dry run no longer advances the dedup census cursor; positive control keeps the real-run rotation honest. Green CI.

@acidkill
acidkill merged commit ee958a4 into acidkill:main Sep 7, 2026
9 checks passed
@acidkill acidkill mentioned this pull request Sep 7, 2026
4 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.

2 participants