Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(torture): unify exec commit and commit crash #790

Merged
merged 1 commit into from
Feb 15, 2025

Conversation

gabriele-0201
Copy link
Contributor

The unification of the two has the side effect of handling
ENOSPC errors also for commits that are expected to crash.

Copy link
Contributor Author

gabriele-0201 commented Feb 7, 2025


// Sample the agent to make sure the changeset was correctly applied or reverted.
let agent_sync_seqn = rr.send_query_sync_seqn().await?;
if snapshot.sync_seqn != agent_sync_seqn {
Copy link
Contributor

Choose a reason for hiding this comment

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

❗️

Shouldn't we expect, for example, that if the commit succeeded then the sync_seqn should actually be equal to snapshot.sync_seqn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we expected that. The current if-else statement doesn't properly cover all expected and unexpected cases. Now that you've let me think more about that, I will update it with:

        // Sample the agent to make sure the changeset was correctly applied or reverted.
        let agent_sync_seqn = rr.send_query_sync_seqn().await?;
        if snapshot.sync_seqn == agent_sync_seqn {
            // The changeset is expected to be applied, the commit succeeded.
            self.ensure_changeset_applied(&rr, &changeset).await?;
            self.state.commit(snapshot);
        } else if self.state.committed.sync_seqn == agent_sync_seqn {
            // The changeset is expected to be reverted, the commit crashed.
            self.ensure_changeset_reverted(&rr, &changeset).await?;
        } else {
            return Err(anyhow::anyhow!("Unexpected agent sync_seqn"));
        }

@gabriele-0201 gabriele-0201 force-pushed the gm_torture_unify_exercise_commit_and_commit_crash branch 3 times, most recently from 560fe73 to f0059bf Compare February 13, 2025 12:14
@gabriele-0201 gabriele-0201 changed the base branch from master to gm_not_increase_sync_seqn_right_away February 13, 2025 12:14
The unification of the two has the side effect of handling
ENOSPC errors also for commits that are expected to crash.
@gabriele-0201 gabriele-0201 force-pushed the gm_torture_unify_exercise_commit_and_commit_crash branch from f0059bf to 5ac99ba Compare February 14, 2025 09:06
@gabriele-0201 gabriele-0201 changed the base branch from gm_not_increase_sync_seqn_right_away to master February 14, 2025 09:07
Copy link
Contributor

pepyakin commented Feb 15, 2025

Merge activity

  • Feb 15, 2:32 AM EST: A user started a stack merge that includes this pull request via Graphite.
  • Feb 15, 2:33 AM EST: A user merged this pull request with Graphite.

@pepyakin pepyakin merged commit 52f95c8 into master Feb 15, 2025
8 checks passed
@pepyakin pepyakin deleted the gm_torture_unify_exercise_commit_and_commit_crash branch February 15, 2025 07:33
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