Skip to content

fix: stop stale tombstones from deleting re-visited sites#7

Merged
sanity merged 2 commits intomainfrom
fix-tombstone-resurrect
Apr 13, 2026
Merged

fix: stop stale tombstones from deleting re-visited sites#7
sanity merged 2 commits intomainfrom
fix-tombstone-resurrect

Conversation

@sanity
Copy link
Copy Markdown
Contributor

@sanity sanity commented Apr 13, 2026

Problem

When a user removed a site and then tried to re-visit it (same session or after a refresh), the site would briefly appear in the sidebar and then vanish before the user could read it. Reported on Matrix by Ivvor:

Another fun issue with Delta related to the problems I've had before. When I try to visit a Delta site that I have previously deleted, Delta deletes it again before I can view it.

Root cause

The `KnownSites` response handler in `ui/src/freenet_api/delegate.rs` applied tombstone records unconditionally:

  1. A legacy delegate could hold a stale removal record for a prefix the user had since re-visited. When legacy migration fired, that tombstone was merged into `REMOVED_PREFIXES` and the live entry was pulled out of `SITES` — even though the current delegate had already responded without that tombstone.
  2. Even the current delegate's own tombstones could race with an in-flight `save_known_sites` write: `visit_site` calls `clear_tombstone` and saves, but a `load_known_sites` response already in flight from before the clear could arrive afterward and yank the placeholder.

The existing `CURRENT_SITES_LOADED` gate only protected legacy real records, not legacy tombstones.

Approach

Extract the tombstone-application decision into a pure function `filter_applicable_tombstones` with two rules:

  1. If the response is from a legacy delegate AND the current delegate has already spoken, drop the legacy tombstone.
  2. If the tombstone's prefix is currently live in `SITES`, drop it regardless of source. Live user intent beats a stale removal record.

The pure-function split makes the logic unit-testable without mocking the delegate runtime or Dioxus signals. The call site now builds a live-prefix set once and delegates the filter decision.

Testing

Six unit tests in `delegate.rs` covering:

  • applies current delegate tombstone when not live (sanity)
  • skips tombstone for a live site even from current delegate (ordering race)
  • skips legacy tombstone when current is authoritative (primary bug)
  • applies legacy tombstone before current loaded (pre-migration path intact)
  • skips legacy tombstone when site is live even before current loads (belt-and-braces)
  • mixed batch partitions correctly

All pass. `cargo fmt` + `cargo clippy -p delta-ui -- -D warnings` clean.

[AI-assisted - Claude]

sanity added 2 commits April 13, 2026 14:25
When a user removed a site, then re-visited it via visit_site (or
re-created it via create_new_site/import_site_key), the site would
briefly appear and then vanish before the user could see it. Root
cause: the KnownSites handler applied tombstones unconditionally, even
legacy-delegate tombstones that had already been superseded by the
current delegate's authoritative response, and even tombstones whose
prefix was currently live in SITES due to an explicit re-add.

Fix: extract the tombstone-application decision into a pure function
filter_applicable_tombstones with two rules:

1. If the response is from a legacy delegate AND the current delegate
   has already responded (CURRENT_SITES_LOADED), drop the legacy
   tombstone. The current delegate is authoritative for the removal
   set.

2. If the tombstone's prefix is currently live in SITES (user just
   re-added), drop it regardless of source. Live intent beats a
   stale removal record. This also closes the race where a
   save_known_sites write hasn't landed yet when an already-in-flight
   load_known_sites response arrives.

Includes 6 unit tests covering the primary bug and its edge cases.

[AI-assisted - Claude]
Adds two unit tests for filter_applicable_tombstones requested by the
PR reviews:

- empty_tombstones_yields_empty_result: sanity case for every flag
  combination, locks down the iterator path against future refactors.
- applies_current_delegate_tombstone_after_current_loaded: the
  production steady-state combination (is_legacy=false,
  current_sites_loaded=true, prefix not live) was not previously
  exercised by any test, even though it's the common case.

Also documents the two new tombstone-application rules in AGENTS.md
under "Known-Sites Tombstone Convention" so the next person touching
the KnownSites response handler preserves them.

[AI-assisted - Claude]
@sanity
Copy link
Copy Markdown
Contributor Author

sanity commented Apr 13, 2026

Review feedback addressed (all 5 reviewers + Codex)

Thanks for the reviews. Pushed 411b35e with the changes below.

Addressed

  • testing-reviewer — Added `empty_tombstones_yields_empty_result` and `applies_current_delegate_tombstone_after_current_loaded`. The production steady-state combination (`is_legacy=false, current_sites_loaded=true`) was not previously exercised; now pinned.
  • big-picture-reviewer — Updated AGENTS.md "Known-Sites Tombstone Convention" section with the two new filter rules so a future agent touching the KnownSites handler preserves them.
  • code-first-reviewer — Documentation notes are covered by the AGENTS.md update and the `filter_applicable_tombstones` docstring already explains the complement-to-`clear_tombstone` relationship.

Acknowledged but not applicable / out of scope

  • skeptical-reviewer H1 (visit_site race): Non-issue on wasm single-threaded — `visit_site` from `clear_tombstone` through `SITES.insert` has no `.await`, so the response handler cannot interleave with it. The native build path is identical in ordering.
  • skeptical-reviewer H2 (raw `has_any`/`has_tombstones` counts): Intentional. The flag means "the current delegate has sent us bytes" rather than "the current delegate has authoritative state usable right now". Flipping it off only when applied tombstones existed would prevent the authoritative-source rule from kicking in for any user whose stored state is only a stale tombstone — that's the exact bug we're fixing. Preserved as-is.
  • skeptical-reviewer H3 (contradictory current-delegate state biases toward deletion): Pre-existing behavior in `restore_known_sites` since before this PR. The `save_known_sites` dedup at line 143 prevents writing contradictions. Out of scope.
  • skeptical-reviewer M1 (no LEGACY_MIGRATION_FIRED timeout): Pre-existing design, not introduced by this PR.
  • skeptical-reviewer M2 (filter/SITES.with_mut not atomic): Wasm single-threaded; the filter read and the subsequent removal are synchronous with no yield.
  • skeptical-reviewer M3 (REMOVED_PREFIXES unbounded growth): Out of scope; would need its own PR.
  • skeptical-reviewer M4 (log wording): Minor; not worth another commit churn.
  • Codex: No discrete bug found; could not complete cargo run due to sandboxed network in its environment. Local tests pass (8/8).

[AI-assisted - Claude]

@sanity sanity merged commit 397300f into main Apr 13, 2026
3 checks passed
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.

1 participant