Skip to content

fix(ext/node): StatementSync.iterate() should reset is_iter_finished flag on every call#31361

Merged
Tango992 merged 3 commits intodenoland:mainfrom
Tango992:fix-node-sqlite-statement-iterator
Nov 20, 2025
Merged

fix(ext/node): StatementSync.iterate() should reset is_iter_finished flag on every call#31361
Tango992 merged 3 commits intodenoland:mainfrom
Tango992:fix-node-sqlite-statement-iterator

Conversation

@Tango992
Copy link
Copy Markdown
Contributor

@Tango992 Tango992 commented Nov 20, 2025

Closes #30144

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 20, 2025 •
edited
Loading

Walkthrough

This PR changes StatementSync.is_iter_finished from bool to Cell<bool> for interior mutability. The field is initialized with Cell::new(false) in database.rs. All read sites use .get() and all write sites use .set(...) in statement.rs, including iterator-reset on creation and when an iterator finishes. A regression test (tests/unit_node/sqlite_test.ts) was added to ensure iterator state is not reused across subsequent iterations after DB mutations.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Caller
  participant StatementSync
  participant DB

  note right of StatementSync `#DDEBF7`: field is_iter_finished: Cell<bool>

  Caller->>StatementSync: prepare() -> returns StatementSync (is_iter_finished = Cell.new(false))
  Caller->>StatementSync: iterate()  --(reset)--> StatementSync: is_iter_finished.set(false)
  loop each next()
    Caller->>StatementSync: next()
    alt rows available
      StatementSync->>DB: fetch row
      DB-->>StatementSync: row
      StatementSync-->>Caller: Some(row)
    else no more rows
      StatementSync-->>StatementSync: is_iter_finished.set(true)
      StatementSync-->>Caller: None
    end
  end
  Caller->>StatementSync: return() (early)
  StatementSync-->>StatementSync: is_iter_finished.set(true)
  StatementSync-->>Caller: Iterator finished
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Files to focus on:
    • ext/node/ops/sqlite/database.rs — correct initialization (Cell::new(false))
    • ext/node/ops/sqlite/statement.rs — all reads use .get() and writes use .set(...); iterator reset paths
    • tests/unit_node/sqlite_test.ts — regression test correctness and coverage of state reset

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically identifies the main change: preventing StatementSync.iterate() from reusing prior iteration state.
Description check ✅ Passed The description references issue #30144, which documents the exact problem being fixed: iterate() reusing previous state on statement reuse.
Linked Issues check ✅ Passed The changes directly address issue #30144 by implementing interior mutability (Cell) to properly reset iterator state, allowing subsequent iterate() calls to reflect current database state.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the iterate() state reuse issue: field type migration, accessor updates, state reset logic, and regression test coverage.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e758f7e and 2208d43.

📒 Files selected for processing (1)
  • ext/node/ops/sqlite/statement.rs (5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: test debug linux-x86_64
  • GitHub Check: test debug macos-aarch64
  • GitHub Check: test debug macos-x86_64
  • GitHub Check: test release linux-x86_64
  • GitHub Check: test debug windows-x86_64
  • GitHub Check: test debug linux-aarch64
  • GitHub Check: build libs
  • GitHub Check: lint debug linux-x86_64
  • GitHub Check: lint debug windows-x86_64
  • GitHub Check: lint debug macos-x86_64
🔇 Additional comments (6)
ext/node/ops/sqlite/statement.rs (6)

3-3: LGTM - necessary import for interior mutability.

The Cell import is required for the Cell<bool> type used in the is_iter_finished field.

77-77: LGTM - interior mutability enables proper state management.

Changing from bool to Cell<bool> allows the iterator closures to mutate the finished flag through shared references, which is exactly what's needed to fix the state reuse issue.

695-709: LGTM - correct usage of Cell for iterator completion.

The flag is checked via .get() and updated via .set(true) when the iterator exhausts. This properly tracks completion state.

737-737: LGTM - proper cleanup on manual iterator close.

Setting the finished flag when the iterator is manually closed via return() is correct.

790-791: LGTM - this is the key fix for the state reuse issue.

Resetting the flag to false at the start of iterate() ensures that each call gets fresh iterator state, fixing the reported bug where subsequent iterations would return stale empty results.

77-77: Verification passed. The database.rs initialization is correct and matches the new Cell<bool> type for is_iter_finished.

Comment @coderabbitai help to get the list of available commands and usage tips.

@Tango992 Tango992 changed the title fix(ext/node): StatementSync.iterate() should not reuse previous state fix(ext/node): StatementSync.iterate() should reset is_iter_finished flag on every call Nov 20, 2025
littledivy
littledivy approved these changes Nov 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM

@Tango992 Tango992 merged commit f61f5ff into denoland:main Nov 20, 2025
21 checks passed
@Tango992 Tango992 deleted the fix-node-sqlite-statement-iterator branch November 20, 2025 16:22
bartlomieju pushed a commit that referenced this pull request Jan 22, 2026
@Tango992 @bartlomieju
fix(ext/node): StatementSync.iterate() should reset `is_iter_finish…
07c3a61 
…ed` flag on every call (#31361)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@littledivy littledivy littledivy approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Sqlite statement reuse with iterate() returns the result from the first call

2 participants

@Tango992 @littledivy