Add completed_event_id FK to prevent session replay #1158

Mshehu5 · 2025-10-09T02:38:13Z

This PR adds completed_event_id foreign key columns to both send_sessions and receive_sessions tables. This column references the exact event row that closed the session.

Closes #1146

Replaced completed_at columns with completed_event_id foreign keys
Updated close() methods to validate if completed_event_id is None
Modified inactive session queries to derive timestamps from closed event

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2025-10-09T02:41:04Z

Pull Request Test Coverage Report for Build 18983609771

Details

33 of 44 (75.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.04%) to 83.542%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/db/error.rs	0	3	0.0%
payjoin-cli/src/db/v2.rs	29	37	78.38%

Totals
Change from base Build 18946545260:	-0.04%
Covered Lines:	9010
Relevant Lines:	10785

💛 - Coveralls

zealsham · 2025-10-09T11:51:23Z

Thank you for taking on this @Mshehu5 . on first pass , this works as intended

spacebear21

The application should just need to properly implement the close() method to set the column appropriately, as close() is called by the payjoin state machine when appropriate; the application should never call save_event() directly. This makes me realize the documentation around the SessionPersister methods needs to be clearer.

payjoin-cli/src/app/v2/mod.rs

payjoin-cli/src/db/v2.rs

arminsabouri

Not a huge fan of using string matching to find the "closed" event and left some other suggestions.

payjoin-cli/src/db/mod.rs

payjoin-cli/src/db/v2.rs

arminsabouri

Its not clear to me what problem the changes in payjoin/src/core/send/v2 and its corresponding change in payjoin-cli are solving. They seem unrelated to the original ticket and should be moved to a different PR once its clear what the problem actually is.

arminsabouri · 2025-10-22T18:43:42Z

payjoin-cli/src/db/v2.rs

-            "UPDATE send_sessions SET completed_at = ?1 WHERE session_id = ?2",
-            params![now(), *self.session_id],
+            "UPDATE send_sessions SET completed_at = ?1, completed_event_id = ?2 WHERE session_id = ?3",
+            params![now(), completed_event_id, *self.session_id],


What happens is completed_event_id is None? There should will always be a session event before close is called. Should we make this expectation explicit?

payjoin-cli/src/app/v2/mod.rs

Mshehu5 · 2025-10-23T17:46:25Z

Its not clear to me what problem the changes in payjoin/src/core/send/v2 and its corresponding change in payjoin-cli are solving. They seem unrelated to the original ticket and should be moved to a different PR once its clear what the problem actually is.

understood, seems the reason I made the change in payjoin/src/core/send/v2 have been addressed in PR #1171 I will take a better approach in issues like this next time

arminsabouri

Just a few more things. Mainly noticing some unrelated changes

arminsabouri · 2025-10-28T14:31:50Z

payjoin-cli/src/db/v2.rs

+            "SELECT r.session_id, e.created_at 
+             FROM receive_sessions r 
+             JOIN receive_session_events e ON r.completed_event_id = e.id 
+             WHERE r.completed_event_id IS NOT NULL",


I don't think this join is neccecary. A session is active until completed_event_id is not null.

You’re right if we’re only fetching inactive sessions, this approach will work. However, from what I’ve observed in the codebase, the get_inactive_send_session_ids() function uses the completed_at timestamp, which is needed for the payjoin-cli history command (see payjoin-cli/src/app/v2, lines 384–409).

The reason for the join is to retrieve the created_at timestamp of the event that closed the session, which serves as the completed_at timestamp.

This is just an observation I might be wrong. If a change is needed to accommodate this, I can include it in this PR or a follow-up one.

arminsabouri · 2025-10-28T14:31:55Z

payjoin-cli/src/db/v2.rs

-            "SELECT session_id, completed_at FROM send_sessions WHERE completed_at IS NOT NULL",
+            "SELECT s.session_id, e.created_at 
+             FROM send_sessions s 
+             JOIN send_session_events e ON s.completed_event_id = e.id 


Same thing here

arminsabouri · 2025-10-31T16:53:25Z

payjoin-cli/src/db/v2.rs

        let conn = self.db.get_connection()?;
        let mut stmt = conn.prepare(
-            "SELECT event_data FROM send_session_events WHERE session_id = ?1 ORDER BY created_at ASC",
+            "SELECT event_data FROM send_session_events WHERE session_id = ?1 ORDER BY id ASC",


Since the ID is always auto-incremeneting, i think this would work. But why make this change? It seems unrelated to original intent of the PR. AFAICT created_at should still be a field on send_session_events

The reason for this change is I noticed events saved within the same second share the same created_at (seconds precision), making created_at non-deterministic and causing out-of-order result

I aslo wanted to add DB transaction to make it more atomic but I think it will be better for a followup PR as it is out of scope

arminsabouri · 2025-10-31T16:54:27Z

payjoin-cli/src/db/v2.rs

-            "SELECT event_data FROM receive_session_events WHERE session_id = ?1 ORDER BY created_at ASC",
+            "SELECT event_data FROM receive_session_events WHERE session_id = ?1 ORDER BY id ASC",


Same thing here.

payjoin-cli/src/db/v2.rs

Add completed_event_id foreign key columns to both send_sessions and receive_sessions tables that reference the exact event that closed the session to replace completed_at column.

zealsham · 2025-10-09T11:55:46Z

payjoin-cli/src/db/v2.rs

+        if matches!(event, SenderSessionEvent::Closed(_)) {
+            conn.execute(
+                "UPDATE send_sessions SET completed_at = ?1, completed_event_id = ?2 WHERE session_id = ?3",
+                params![now(), event_id, *self.session_id],


The time here would be inacurrate for when the actual session event was created . No need to call now() again here . Rather use the time when the actual session_event was created as this just tells us the time this DB entry was made

Thanks for the review Zealsham!
It seems your comments are based on an older version of this PR. I’d really appreciate it if you could take a look at the latest changes as well.

Mshehu5 marked this pull request as ready for review October 9, 2025 09:45

spacebear21 requested changes Oct 9, 2025

View reviewed changes

payjoin-cli/src/app/v2/mod.rs Outdated Show resolved Hide resolved

payjoin-cli/src/db/v2.rs Outdated Show resolved Hide resolved

Mshehu5 force-pushed the completed_at_fk branch 3 times, most recently from c0fd47a to de46c09 Compare October 10, 2025 22:07

DanGould requested a review from arminsabouri October 13, 2025 17:43

arminsabouri requested changes Oct 13, 2025

View reviewed changes

payjoin-cli/src/db/mod.rs Outdated Show resolved Hide resolved

payjoin-cli/src/db/mod.rs Outdated Show resolved Hide resolved

payjoin-cli/src/db/v2.rs Outdated Show resolved Hide resolved

Mshehu5 force-pushed the completed_at_fk branch 2 times, most recently from fbe10e6 to 4866994 Compare October 15, 2025 18:20

Mshehu5 requested a review from arminsabouri October 18, 2025 14:15

Mshehu5 force-pushed the completed_at_fk branch from 4866994 to b7c8741 Compare October 22, 2025 10:24

arminsabouri requested changes Oct 22, 2025

View reviewed changes

Mshehu5 force-pushed the completed_at_fk branch 4 times, most recently from dad306d to f02b04b Compare October 27, 2025 16:04

Mshehu5 requested a review from arminsabouri October 27, 2025 16:09

Mshehu5 force-pushed the completed_at_fk branch from f02b04b to 622a6c6 Compare October 27, 2025 16:12

arminsabouri requested changes Oct 31, 2025

View reviewed changes

Add completed_event_id FK to prevent session replay

37aa822

Add completed_event_id foreign key columns to both send_sessions and receive_sessions tables that reference the exact event that closed the session to replace completed_at column.

Mshehu5 force-pushed the completed_at_fk branch from 622a6c6 to 37aa822 Compare October 31, 2025 19:51

zealsham reviewed Nov 2, 2025

View reviewed changes

		"SELECT event_data FROM receive_session_events WHERE session_id = ?1 ORDER BY created_at ASC",
		"SELECT event_data FROM receive_session_events WHERE session_id = ?1 ORDER BY id ASC",

Add completed_event_id FK to prevent session replay #1158

Are you sure you want to change the base?

Add completed_event_id FK to prevent session replay #1158

Uh oh!

Conversation

Mshehu5 commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 18983609771

Details

💛 - Coveralls

Uh oh!

zealsham commented Oct 9, 2025

Uh oh!

spacebear21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Mshehu5 commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arminsabouri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mshehu5 commented Oct 9, 2025 •

edited

Loading

coveralls commented Oct 9, 2025 •

edited

Loading

Mshehu5 commented Oct 23, 2025 •

edited

Loading