Use time-based uuid instead of raw increment for session ids

[benalleng]

Closes #1631

This is a demonstration of the session ids being time based uuids in our reference implementation instead of a raw incremented integer

With the nature of seconds based time + a uuid I think that this is the best way for both user initiated and high frequency payjoin session management should be handled.

This is a breaking change in the database for any payjoin-cli with existing sessions

Coded up by deepseek v4

Pull Request Checklist

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]

Coverage Report for CI Build 27421136182

Coverage increased (+0.003%) to 85.191%

Details

• Coverage increased (+0.003%) from the base build.
• Patch coverage: 7 uncovered changes across 2 files (30 of 37 lines covered, 81.08%).
• 1 coverage regression across 1 file.

Uncovered Changes

File	Changed	Covered	%
payjoin-cli/src/db/v2.rs	28	22	78.57%
payjoin-cli/src/app/v2/mod.rs	2	1	50.0%
Total (4 files)	37	30	81.08%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File	Lines Losing Coverage	Coverage
payjoin-cli/src/app/v2/mod.rs	1	51.0%

---

Coverage Stats

Relevant Lines:	14728
Covered Lines:	12547
Line Coverage:	85.19%
Coverage Strength:	368.79 hits per line

---

💛 - Coveralls

[zealsham]

tAck,
I notice that except the current database sqlite file is deleted no payjoin-cli command will work which makes sense since we change the datatype of a particular column.
That makes this a breaking change as it becomes impossible to complete any pending session.
Sessions expire in 24hr which makes it worth the tradeoff anyways.

[Arowolokehinde]

One thing I noticed while reviewing is that the (true, true) branch at v2/mod.rs:540 was added to handle the AUTOINCREMENT ID collision. Since this PR fixes that root cause, the branch is now unreachable. Should it be cleaned up, or is it still useful as a defensive guard?

[benalleng]

Since this PR fixes that root cause, the branch is now unreachable. Should it be cleaned up, or is it still useful as a defensive guard?

Good find, and while the possibility of collision is not zero it is effectively zero so I think it's worth reconsidering needing to pass the --role flag in the cancel command with this logic, though I think it's appropriate for a follow-up.

[benalleng]

Making a note that this is a breaking change

[nothingmuch]

Making a note that this is a breaking change

this seems weird to me... we're before the release and have explicitly broken the schema multiple times

also, the whole idea of payjoin-cli is that it's a standalone thing, its database code hasn't been extracted to a standalone crate yet, and the trait is designed to be agnostic to the kind of ID

v7 UUIds make a lot of sense as an ID type for anything where less coordination is desired, and sequential IDs make sense for a single database. sqlite being embedded, the latter is probably a good fit for sqlite based instances except in some circumstances.

what is the actual motivation for changing payjoin-cli? time based UUIDs are appropriate but are overkill for the specific kind of implementation that payjoin-cli has so to me this change seems like it adds confusion.

in a standalone payjoin-session-db or something, i think having a choice or defaulting to time based UUIDs does make sense but this seems to be addressing a problem in payjoin-cli that exists specifically in not-payjoin-cli implementations

[DanGould]

We're making this change so that payjoin-cli is a robust reference to influence implementations of those actually referencing it in integration rather than because it's the simplest thing for this environment. It lets us get rid of --role too, which is a nice ergonomic plus.

#1631 (comment)

[nothingmuch]

We're making this change so that payjoin-cli is a robust reference to influence implementations of those actually referencing it in integration rather than because it's the simplest thing for this environment.

adding cross cutting concerns to a reference implementation that seem to run contrary to its other implementation choices does not, it adds confusion and encourages cargo cult programming

that's not to say that v7 UUIDs don't make sense for this use case, they do, just not if sqlite is the underlying database

It lets us get rid of --role too, which is a nice ergonomic plus.

that is completely unrelated and can be solved without changing the unique ID domain from sequentially assigned numbers to randomly assigned ones

[nothingmuch]

that's not to say that v7 UUIDs don't make sense for this use case, they do, just not if sqlite is the underlying database

especially when the same database backend still specifies locking_mode = EXCLUSIVE

[DanGould]

that's not to say that v7 UUIDs don't make sense for this use case, they do, just not if sqlite is the underlying database

especially when the same database backend still specifies locking_mode = EXCLUSIVE

I see now that UUID v7 is suboptimal for this particular implementation and for my communication goals. If the goal is to let correct, robust implementations stand up as fast as possible it makes more sense to show best practice here with the rationale and an explanation of what to do in other circumstances.

Here, sequential IDs are even more ergonomic (payjoin-cli cancel 3 versus payjoin-cli cancel 0191a4f2-7c3e-7b9a-...) and they wouldn't even need to be random if send/receive sessions just shared a table (either with nullable columns or session_id being a foreign key from its own table) which also works because SQLite serializes writers. It makes sense to use a v7 UUID where there's multiple independent stores, write-authorities, or if impls want to generate the id before insert.

The simple way to do this seems to be nullable columns like this:

sessions(
  session_id    INTEGER PRIMARY KEY AUTOINCREMENT,
  role          TEXT NOT NULL,        -- 'send' / 'receive'
  pj_uri        TEXT,                 -- NULL for receive
  receiver_pubkey BLOB,              -- NULL for receive
  completed_at  INTEGER
)

while making the nullable row shape unrepresentable in Rust by making role the discriminant of a small enum.

We could use a foreign key instead of nullable columns to enforce at the schema, but that does make the schema more complicated and introduces inserts to multiple tables per write. a CHECK constraint keeps the DB honest without a second table.

What I now see as really important after this discussion is to document the rationale in a number of places to prevent cargo cult programming by humans or agents:

first, on the SessionId struct

/// sequential i64 because a SQLite database addressed by a human at the cancel prompt
/// - only ever has one writer and will therefore not conflict
/// - is easier to type on cli than a random string which may be required by independent writers
SessionId(i64)

Second, on init_schema / in it next to the sessions table definition stating that ids are unique across both roles which allows cancel to work without --role disambiguation.

Third on the SessionPersister trait saying that the implementer decides session identification, the library imposes no id scheme, and that sequential for a single write-authority vs time-ordered v7 UUID for independent writers / pre-insert generation / to avoid coordinating a shared counter.