Skip to content

fix(delete_by_expiry): add missing bind parameter and handle string-to-number conversion#98

Merged
genusistimelord merged 3 commits intoAscendingCreations:mainfrom
NCura:fix(delete_by_expiry)-properly-handle-string-to-number-conversion
Oct 14, 2025
Merged

fix(delete_by_expiry): add missing bind parameter and handle string-to-number conversion#98
genusistimelord merged 3 commits intoAscendingCreations:mainfrom
NCura:fix(delete_by_expiry)-properly-handle-string-to-number-conversion

Conversation

@NCura
Copy link
Contributor

@NCura NCura commented Oct 14, 2025

Problem

The delete_by_expiry function had two bugs that prevented it from cleaning up expired sessions:

  1. Missing bind parameter: The SELECT query referenced $expires but never bound it, causing the query
    to return empty results
  2. Type mismatch: When sessionexpires is stored as a string (e.g., '1760389854'), the comparison
    sessionexpires < $expires fails because SurrealDB compares string-to-number incorrectly

Solution

  • Added the missing .bind(("expires", now)) to the SELECT query
  • Wrapped sessionexpires with type::number() in both SELECT and DELETE queries to ensure proper numeric comparison

Testing

Tested in my project with thousands of expired sessions that were not being cleaned up. After the fix, all expired sessions are now properly deleted.

Optional Further Optimization

This fix maintains the current two-query approach (SELECT + DELETE). However, SurrealDB supports RETURN BEFORE in DELETE statements, which would allow combining both queries into one:

let mut res = self.connection
    .query("DELETE type::table($table_name)
           WHERE sessionexpires = NONE OR type::number(sessionexpires) < $expires
           RETURN BEFORE;")
    .bind(("table_name", table_name.to_string()))
    .bind(("expires", now))
    .await
    .map_err(|err| DatabaseError::GenericSelectError(err.to_string()))?;

let records: Vec<SessionRecord> = res.take(0).map_err(|err| DatabaseError::GenericSelectError(err.to_string()))?;
let ids: Vec<String> = records.into_iter().map(|r| r.sessionid).collect();

Let me know if you'd like me to include this optimization in the PR as well.

Copy link
Member

@genusistimelord genusistimelord left a comment

Choose a reason for hiding this comment

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

thank you for catching and fixing this issue.

@genusistimelord
Copy link
Member

if that Optimization is indeed an optimizations I don't see why you would not just include it?

@NCura
Copy link
Contributor Author

NCura commented Oct 14, 2025

I didn't want to mix the fix with the optimization in this PR, and I was not sure if there was a good reason to separate the query into SELECT + DELETE. I've now included the change in the last commit.

Copy link
Member

@genusistimelord genusistimelord left a comment

Choose a reason for hiding this comment

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

approved the change. thank you

@genusistimelord genusistimelord merged commit ddd55a3 into AscendingCreations:main Oct 14, 2025
3 checks passed
@NCura NCura deleted the fix(delete_by_expiry)-properly-handle-string-to-number-conversion branch October 15, 2025 07:30
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

Comments