Skip to content

fix: enforce MessageDeduplicator count bounds for marked IDs - #1635

Open
vekovius wants to merge 3 commits into
permissionlesstech:mainfrom
vekovius:codex/fix-message-deduplicator-bound
Open

fix: enforce MessageDeduplicator count bounds for marked IDs#1635
vekovius wants to merge 3 commits into
permissionlesstech:mainfrom
vekovius:codex/fix-message-deduplicator-bound

Conversation

@vekovius

@vekovius vekovius commented Aug 5, 2026

Copy link
Copy Markdown

What changed

I added the existing expiry cleanup before markProcessed(_:) checks the ID, then apply count trimming after a new insertion. Duplicate marks remain a no-op.

I also added an injectable clock with Date.init as the production default, matching the existing repository pattern. The expiry regression can now advance time directly instead of sleeping against the wall clock.

Why

isDuplicate(_:) already trims after insertion, but markProcessed(_:) did not. BLEService shares one deduplicator across both entry points, so marked IDs could overshoot the configured maxCount between a markProcessed(_:) call and the next isDuplicate(_:) or explicit cleanup. This change makes markProcessed(_:) enforce the same bound immediately.

One regression uses a maximum count of four and inserts five IDs. It fails before this change because all five remain present; after the fix, the existing 75% trim policy removes the two oldest IDs and retains the newest three.

The second regression mixes an expired ID with four current IDs. It verifies that the expired entry is removed before count trimming can evict a still-valid ID. Both tests use a deterministic clock.

Tests

  • swift test --filter MessageDeduplicatorTests — 2 tests passed
  • swift test --quiet — 2,006 tests passed

This is one of four independent findings from an audit of main. It does not depend on the other changes and can be reviewed or merged separately.

Related independent findings

@vekovius
vekovius marked this pull request as ready for review August 5, 2026 17:44
Copilot AI lite review requested due to automatic review settings August 5, 2026 17:44

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a capacity-bound inconsistency in MessageDeduplicator by ensuring IDs added via markProcessed(_:) also trigger the existing count-based trimming policy, preventing unbounded growth when markProcessed(_:) is used independently of isDuplicate(_:).

Changes:

  • Call trimIfNeeded() after inserting a new ID in markProcessed(_:).
  • Add a regression test that verifies markProcessed(_:) enforces maxCount using the existing 75% trim policy.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
bitchat/Utils/MessageDeduplicator.swift Ensures markProcessed(_:) enforces maxCount by trimming after insertion.
bitchatTests/MessageDeduplicatorTests.swift Adds a regression test asserting the oldest IDs are trimmed when exceeding maxCount.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread bitchat/Utils/MessageDeduplicator.swift

@Chessing234 Chessing234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the gap is real, markProcessed did skip the trim.

two things. the pr says these ids grow past maxCount, but BLEService shares one deduplicator for both entry points, so the next isDuplicate trims the same store — the reachable case is overshoot between calls, not unbounded growth. worth stating accurately.

and markProcessed_cleansExpiredEntriesBeforeCountTrim leans on a 200ms real sleep against maxAge 0.1, which will flake on a loaded runner. the house pattern is an injected clock (BLEIncomingFileStore.swift:170, dateProvider: @escaping () -> Date = Date.init) — adding one here makes both new tests deterministic.

@vekovius

Copy link
Copy Markdown
Author

@Chessing234 Addressed in 97f94e7. I corrected the description to the reachable overshoot between shared-store calls and added the repository dateProvider pattern. Both new tests are deterministic now; the 2 focused tests and the full 2,006-test suite pass.

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.

3 participants