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

manually drop snapshot without running its destructor #249

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

runsisi
Copy link
Contributor

@runsisi runsisi commented Jul 31, 2024

this fixes the panic in debug build:

thread 'tokio-runtime-worker' panicked at crates/corro-types/src/agent.rs:1306:9:
max value was not applied

steps to reproduce:

  • schema

    CREATE TABLE todos ( id INTEGER NOT NULL PRIMARY KEY, title TEXT NOT NULL DEFAULT '' );

  • node A

    ./corrosion -c corro.toml exec 'INSERT INTO todos (id, title) VALUES (1, "test todo")'

  • node B

    ./corrosion -c corro.toml query 'SELECT * FROM todos' ./corrosion -c corro.toml exec 'DELETE FROM todos WHERE id == 1'

now node A crashes with assert message max value was not applied

Copy link
Member

@jeromegn jeromegn left a comment

Choose a reason for hiding this comment

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

The fix for the panic might be to std::mem::forget(snap). it's a little weird all this all worked out, but we couldn't hold a write lock across the commit or else we'd keep the lock for far too long and some of our tests started failing.

snap.update_cleared_ts(&tx, ts)
.map_err(|source| ChangeError::Rusqlite {
source,
actor_id: None,
version: None,
})?;

booked.commit_snapshot(snap);
Copy link
Member

Choose a reason for hiding this comment

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

Running this before we know the transaction succeeds can leave the in-memory bookkeeping in a desynchornized state, compared to the database.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it. thanks!

@runsisi runsisi force-pushed the wip-fix-snap branch 2 times, most recently from 8a9eb6d to cf878ad Compare August 1, 2024 06:02
@runsisi runsisi changed the title commit snapshot before dropping manually drop snapshot without running its destructor Aug 1, 2024
this fixes the panic in debug build:

  thread 'tokio-runtime-worker' panicked at crates/corro-types/src/agent.rs:1306:9:
  max value was not applied

steps to reproduce:

  - schema

    CREATE TABLE todos (
        id INTEGER NOT NULL PRIMARY KEY,
        title TEXT NOT NULL DEFAULT ''
    );

  - node A

    ./corrosion -c corro.toml exec 'INSERT INTO todos (id, title) VALUES (1, "test todo")'

  - node B

    ./corrosion -c corro.toml query 'SELECT * FROM todos'
    ./corrosion -c corro.toml exec 'DELETE FROM todos WHERE id == 1'

  now node A crashes with assert message `max value was not applied`

Signed-off-by: runsisi <[email protected]>
@jeromegn jeromegn merged commit 9ae22b2 into superfly:main Aug 1, 2024
3 checks passed
@runsisi runsisi deleted the wip-fix-snap branch August 1, 2024 14:02
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