fix: Always restore snapshot after solutions#305
Conversation
There was a problem hiding this comment.
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_iterationto 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.
| // 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, | ||
| } { |
There was a problem hiding this comment.
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.
8e9baca to
eb87c15
Compare
There was a problem hiding this comment.
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_iterationincrementsself.iterationsfor every stop reason (including solutions/timeouts), butshould_restore_snapshot_this_iteration()usesself.iterationsto 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 whensnapshot_restore_modeisPolicyControlled) or changingshould_restore_snapshot_this_iterationto 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
iterationscounter 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.
src/interfaces/fuzz.rs
Outdated
| /// it had finished executing with an exception or timeout, and the initial snapshot | ||
| /// will always be restored before the next iteration resumes. |
There was a problem hiding this comment.
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.
| /// 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. |
src/lib.rs
Outdated
| /// 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, |
There was a problem hiding this comment.
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.
eb87c15 to
47528a2
Compare
No description provided.