Skip to content

fix: Arc::relocated() storage corruption when source == destination#310

Merged
Darksecond merged 3 commits intomainfrom
copilot/improve-thread-aware-arc-relocation
Mar 10, 2026
Merged

fix: Arc::relocated() storage corruption when source == destination#310
Darksecond merged 3 commits intomainfrom
copilot/improve-thread-aware-arc-relocation

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 10, 2026

Arc::relocated() corrupts shared storage when source == destination: it stores the factory-created value at the destination slot, then unconditionally overwrites that same slot with the stale pre-relocation value. Any subsequent relocated() call hits the get_clone fast-path and silently returns the wrong data.

Changes

  • Bug fix: Guard the source-slot restore with if source != destination — skips the overwrite when both affinities refer to the same slot.
  • Regression test: test_relocated_source_equals_destination_does_not_corrupt_storage — advances a counter to 42, relocates with source == destination (factory resets to 0), then performs a second relocation; without the fix the second call would return 42 via the corrupted fast-path.
// Before fix: storage at `affinity` ends up holding the stale Counter(42),
// so the second relocated() returns 42 instead of 0.
let arc = PerCore::new(Counter::new);
arc.increment_by(42);

let relocated = arc.relocated(affinity.into(), affinity); // value = 0 (new), storage = 42 (stale) ← bug
let again = relocated.relocated(affinity.into(), affinity); // returns 42 ← wrong

// After fix: storage holds 0, second call returns 0.
Original prompt

This section details on the original issue you should resolve

<issue_title>Improve the soundness of relocating thread_aware::Arc when multiple sources are provided</issue_title>
<issue_description>I have encountered an issue when relocating thread_aware::Arc and unknowingly providing multiple different source affinities. This caused a bug where I had a "dead instance" caused by multiple relocations.

The problem disappeared where I made sure that only a single source affinity was provided across all relocations calls. Would be nice to provide some debug assertion to ensure caller provides a proper source affinity.
</issue_description>

Comments on the Issue (you are @copilot in this section)

@Darksecond Bug: thread_aware::Arc::relocated() corrupts storage when source == destination

In Arc::relocated() (cell/mod.rs line 488), when the source and destination affinities refer to the same slot, the method:

  1. Creates a new value via factory and stores it at the destination slot (line 524)
  2. Unconditionally stores the pre-relocation self.value at the source slot (line 534) — which is the same slot — overwriting the value just created in step 1

The returned Arc correctly holds the factory-created value, but the shared storage now holds the stale pre-relocation value. Any subsequent relocated() call
targeting that affinity finds the stale value via get_clone(destination) (line 492), silently returning the wrong data.

Fix: Skip the source store when source equals destination:

if let MemoryAffinity::Pinned(source) = source {
if source != destination {
guard.replace(source, self.value);
}
}

Workaround: Callers can pass MemoryAffinity::Unknown as source to bypass the source-store path entirely (the if let Pinned guard is false for Unknown).</comment_new>


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

@Darksecond Darksecond changed the title [WIP] Add debug assertion for thread_aware::Arc relocation fix: [WIP] Add debug assertion for thread_aware::Arc relocation Mar 10, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 10, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (515d4ce) to head (a8df600).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #310   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         152      152           
  Lines        9335     9337    +2     
=======================================
+ Hits         9335     9337    +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…dd debug_assert for inconsistent source affinities

Co-authored-by: Darksecond <291582+Darksecond@users.noreply.github.com>
Copilot AI changed the title fix: [WIP] Add debug assertion for thread_aware::Arc relocation Fix Arc::relocated() storage corruption when source == destination Mar 10, 2026
@Darksecond Darksecond marked this pull request as ready for review March 10, 2026 08:54
Copilot AI review requested due to automatic review settings March 10, 2026 08:54
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes a storage corruption bug in thread_aware::Arc::relocated() that occurs when the relocation source and destination refer to the same pinned slot, and adds a regression test to prevent recurrence.

Changes:

  • Prevent overwriting the destination slot with the stale pre-relocation value when source == destination.
  • Add a debug_assert! intended to catch inconsistent source-affinity usage for closure factories.
  • Add a regression test covering the source == destination corruption scenario.

Reviewed changes

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

File Description
crates/thread_aware/src/cell/mod.rs Avoids restoring the source slot when it aliases the destination; adds a debug assertion around closure-factory relocation source consistency.
crates/thread_aware/src/cell/tests.rs Adds a regression test ensuring repeated relocation with source == destination does not corrupt storage / fast-path results.

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

You can also share your feedback on Copilot code review. Take the survey.

Comment thread crates/thread_aware/src/cell/mod.rs Outdated
@Darksecond Darksecond enabled auto-merge (squash) March 10, 2026 09:13
Comment thread crates/thread_aware/src/cell/tests.rs
@Darksecond Darksecond disabled auto-merge March 10, 2026 09:27
@Darksecond
Copy link
Copy Markdown
Contributor

Please remove the debug_assert! @copilot

Co-authored-by: Darksecond <291582+Darksecond@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 10, 2026

Please remove the debug_assert! @copilot

Done — removed the debug_assert! in a8df600.

@Darksecond Darksecond changed the title Fix Arc::relocated() storage corruption when source == destination fix: Arc::relocated() storage corruption when source == destination Mar 10, 2026
@Darksecond Darksecond enabled auto-merge (squash) March 10, 2026 10:03
@Darksecond Darksecond merged commit efe4ae7 into main Mar 10, 2026
29 of 45 checks passed
@Darksecond Darksecond deleted the copilot/improve-thread-aware-arc-relocation branch March 10, 2026 12:29
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.

Improve the soundness of relocating thread_aware::Arc when multiple sources are provided

5 participants