Skip to content

fix: Always restore snapshot after solutions#305

Open
Wenzel wants to merge 1 commit intointel:mainfrom
Wenzel:fix/always-restore-on-solution
Open

fix: Always restore snapshot after solutions#305
Wenzel wants to merge 1 commit intointel:mainfrom
Wenzel:fix/always-restore-on-solution

Conversation

@Wenzel
Copy link
Contributor

@Wenzel Wenzel commented Mar 9, 2026

No description provided.

Copy link

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

This PR ensures the simulator always restores the initial snapshot after solution/timeout stops, rather than relying solely on the configured snapshot-restore interval policy, improving iteration reliability after crashes/timeouts.

Changes:

  • Introduces a stop-specific snapshot restore mode (SnapshotRestoreMode) to distinguish policy-controlled restores from unconditional restores.
  • Extends finish_iteration to accept the restore mode and applies it when deciding whether to restore.
  • Uses unconditional restore (Always) for solution/timeout stops, while keeping policy-controlled behavior for normal/manual stops.

💡 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 on lines +164 to +168
// 5) Restore to initial snapshot according to the stop-specific restore policy.
if match snapshot_restore_mode {
SnapshotRestoreMode::PolicyControlled => self.should_restore_snapshot_this_iteration(),
SnapshotRestoreMode::Always => true,
} {
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The new SnapshotRestoreMode::Always behavior means solution/timeout stops will restore the initial snapshot even when snapshot_restore_interval is configured to never/periodic restore. That appears to contradict the existing interface documentation that says solution() restores according to snapshot_restore_interval (see src/interfaces/fuzz.rs docs). Please update the relevant docs (and/or config semantics) to match this new stop-specific restore policy so users aren’t misled.

Copilot uses AI. Check for mistakes.
@Wenzel Wenzel force-pushed the fix/always-restore-on-solution branch 2 times, most recently from 8e9baca to eb87c15 Compare March 9, 2026 14:36
@Wenzel Wenzel requested a review from Copilot March 9, 2026 14:56
Copy link

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/haps/mod.rs:119

  • finish_iteration increments self.iterations for every stop reason (including solutions/timeouts), but should_restore_snapshot_this_iteration() uses self.iterations to decide policy-controlled restores. With the updated semantics/docs that the interval applies to non-solution iterations, solution iterations currently shift the restore cadence. Consider tracking a separate counter for non-solution iterations (only incremented when snapshot_restore_mode is PolicyControlled) or changing should_restore_snapshot_this_iteration to accept an explicit counter.
        // 1) Count this iteration as complete.
        self.iterations += 1;

docs/src/config/common-options.md:216

  • This section says the default behavior is restore at every non-solution boundary, but the example comment still reads "restore every iteration" and the later text implies the interval counts only "normal iterations". Given the current implementation uses a single iterations counter that includes solutions/timeouts, please adjust the wording/example to match the actual cadence (or update the implementation accordingly).
By default, TSFFS restores the initial snapshot at every non-solution iteration boundary.
This can be changed to support semi-persistent or fully persistent execution between normal
iterations:

```python
# Default behavior: restore every iteration
@tsffs.snapshot_restore_interval = 1

💡 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 on lines +240 to +241
/// it had finished executing with an exception or timeout, and the initial snapshot
/// will always be restored before the next iteration resumes.
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The doc comment says the initial snapshot will "always" be restored after calling solution(), but if a solution is signaled before the initial snapshot is taken the implementation resumes without restoring (see on_simulation_stopped_solution). Please qualify this as "if an initial snapshot exists" or document that solution() must only be called after startup snapshotting.

Suggested change
/// it had finished executing with an exception or timeout, and the initial snapshot
/// will always be restored before the next iteration resumes.
/// it had finished executing with an exception or timeout, and, if an initial
/// snapshot exists, it will be restored before the next iteration resumes.

Copilot uses AI. Check for mistakes.
src/lib.rs Outdated
Comment on lines 254 to 262
/// Snapshot restore policy between non-solution iterations.
///
/// Accepted values:
/// - `1` restores on every iteration (default)
/// - `N > 1` restores every N iterations
/// - `1` restores on every non-solution iteration (default)
/// - `N > 1` restores every N non-solution iterations
/// - `0` disables restores after startup
///
/// Solution iterations always restore the initial snapshot before resuming.
pub snapshot_restore_interval: SnapshotRestorePolicy,
Copy link

Copilot AI Mar 9, 2026

Choose a reason for hiding this comment

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

The struct field docs now state restores occur every N non-solution iterations and that solutions always restore. The current restore cadence logic is based on self.iterations (which includes solution/timeouts), so either the docs should clarify that solutions still advance the interval counter or the implementation should be updated to count only non-solution iterations for the interval policy.

Copilot uses AI. Check for mistakes.
@Wenzel Wenzel force-pushed the fix/always-restore-on-solution branch from eb87c15 to 47528a2 Compare March 9, 2026 16:00
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