Skip to content

Scope SQLite control argument lifetime - #37

Merged
simolus3 merged 2 commits into
powersync-ja:mainfrom
GuionAI:fix/copy-dynamic-sqlite-control-arguments
Sep 16, 2026
Merged

simolus3 merged 2 commits into
powersync-ja:mainfrom
GuionAI:fix/copy-dynamic-sqlite-control-arguments

Conversation

@birdmanmandbir

@birdmanmandbir birdmanmandbir commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Keep borrowed control arguments bound with STATIC; SQLite does not need to copy them.
  • Mark the binding helper unsafe and document its lifetime requirement.
  • Explicitly finalize the statement before its owned control argument leaves scope.

Root cause

The owned argument was declared after the statement, so normal Rust drop order could destroy it first. STATIC bindings require the backing value to outlive the statement.

Validation

  • cargo fmt --all -- --check
  • cargo test -p powersync --all-features
  • cargo clippy -p powersync --all-targets -- -D warnings

Fixes #35

@simolus3 simolus3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't agree with this, and the test doesn't reproduce how PowerSyncControlArgument is actually used in invoke_control.

Semantically, we want to borrow the string to the SQLite statement, and the static destructor is acceptable for this. To make this cleaner rust, we could

  1. Mark bind_to as unsafe, and add a safety comment stating that the reference must outlive the statement.
  2. Explain that in invoke_control, where we'd explicitly drop the statement before arguments.

@birdmanmandbir birdmanmandbir changed the title Copy dynamic SQLite control arguments Scope SQLite control argument lifetime Sep 16, 2026
@birdmanmandbir

Copy link
Copy Markdown
Contributor Author

Addressed in b0c1606: this keeps borrowed STATIC bindings, makes bind_to unsafe with its lifetime contract, and explicitly drops the statement before the argument can leave scope. I also removed the regression test because it bound a temporary rather than exercising invoke_control's real lifetime.

cargo fmt --all -- --check, cargo test -p powersync --all-features, and cargo clippy -p powersync --all-targets -- -D warnings pass.

@simolus3
simolus3 merged commit 0b83ee9 into powersync-ja:main Sep 16, 2026
2 checks passed
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.

Copy dynamic SQLite control arguments before their owner can drop

2 participants