Match a document by several identifiers

[pid1]

PUT /syncs/progress takes an optional identifiers array and GET /syncs/progress/:document an optional ids query parameter, so a renamed or recompressed copy can find the position stored for another copy.

A request that names none behaves exactly as before: same code path, same response. config/routes.lua is unchanged and there is no new API version.

Identifiers are { "type", "value" } pairs in the client's order of preference, and the first must equal document. A type is an opaque label the server stores and echoes without interpreting, so new identifiers need no server change.

GET /syncs/progress/<content>?ids=content:<d>,structure:<d>,metadata:<d>

{"document":"<the digest the record is stored under>", "progress":"<xpointer>",
 "percentage":0.42, "device":"my kpw", "timestamp":1758472800,
 "match":"structure", "progress_match":"structure"}

Two fields come back, and only for a request that named identifiers. match is the identifier type that resolved the lookup. progress_match is the strongest identifier shared with whoever wrote the current progress string, and is the one that says whether the xpointer can be followed. They differ when a weaker identifier let another copy write into the record, which is the case where a reader should seek by percentage instead.

Identifiers other than the record's own become aliases, per account, removed with the account. An alias is only created, never repointed, and never shadows a document that exists in its own right, so a weak identifier can fail to match but cannot move a position onto the wrong record. At most 8 per request.

The server sees only digests: no title, no author, no filename.

65 tests pass. spec/controllers/1/syncs_controller_spec.lua is untouched, so the existing suite is the guard that a request naming no identifiers is unaffected.

---

This change is 

[Frenzie]

I'll take a look at it later, but in the meantime tell Claude to be more succinct in its comments. Could you briefly sketch what it actually looks like from the API side?

[pid1]

PUT is unchanged apart from an identifiers list, in the client's order of preference. The first entry must equal document.

{"document":"<content>",
 "identifiers":[{"type":"content","value":"<content>"},
                {"type":"structure","value":"<structure>"},
                {"type":"metadata","value":"<metadata>"}],
 "percentage":0.42,"progress":"/body/DocFragment[20]/body/p[22]","device":"my kpw"}

A GET has no body, so the same list goes in one query parameter:

GET /syncs/progress/<content>?ids=content:<d>,structure:<d>,metadata:<d>
Accept: application/vnd.koreader.v2+json

The response is v1 plus two fields:

{"document":"<the digest the record is stored under>", "percentage":0.42,
 "progress":"/body/DocFragment[20]/body/p[22]", "device":"my kpw", "timestamp":1758472800,
 "match":"content", "progress_match":"metadata"}

match is how the lookup resolved. progress_match is the strongest identifier the reader shares with whoever wrote that progress string. They differ when a weaker alias let another copy write into the record, which is when the xpointer should not be followed.

Version comes from Accept, so v1 clients never see any of it.

[Frenzie]

Alright, thanks.

Is it correct that this version of v2 is missing the equivalent of #56?

[pid1]

Yes, it is missing. v2 checks document with is_valid_key_field only, and its read route is the same /syncs/progress/:document binding, so it strands a write exactly as v1 does.

Identifier values are not affected: they travel in the ids query parameter rather than the path. And the record is always keyed by the first identifier, which has to equal document, so guarding document covers them.

I would rather rebase this on #56 than duplicate the check here, once you have decided about that one. Say which and I will push it.

[pid1]

Rebased on master and v2 now has the same guard, as its own commit. 66 tests pass, of which 28 are the master suite unchanged.

The helper is duplicated rather than lifted somewhere shared, the way is_valid_key_field already is in that file, so nothing in v1 had to change for v2 to have it.

[pid1]

all set

[Frenzie]

Thanks. The text in the README could also be (much) briefer.

Incidentally, did you run any kind of performance benchmarks to get a rough idea of the impact of the additional options?

[pid1]

Sure, I'll attack both. No, I didn't do any performance testing, but maybe we can use the conformance test for that? I'll give it a shot, update here with performance testing results, and, if its worth it, I'll push another PR that can do CI performance regression tests.

[pid1]

Benchmarked. This is a laptop, so the percentages are the useful part and the absolute rates are not.

Method: both trees built from source against the same OpenResty 1.29.2.3, the same rock tree and the same redis over a unix socket. Arms interleaved across five repetitions, FLUSHDB and a seed write before every measurement, 3s warmup discarded each time. wrk at 4 threads and 8 connections for throughput, 1 connection for service time. CONFIG RESETSTAT and INFO commandstats around each measured run. Roughly 400k requests per arm. Each percentage is the mean of five ratios against v1 on master paired within the same repetition. Between-repetition CV was 0.4 to 1.6% and there were no errors or non-2xx responses in any of the 100 measurements.

arm	PUT	GET	redis commands per request, PUT / GET
v1 on master	8429/s	8600/s	6 / 4
v1 on this branch	-0.1%	+0.4%	6 / 4
v2, 1 identifier	-4.2%	-6.1%	7 / 6
v2, 3 identifiers	-5.4%	-6.1%	13 / 6
v2, 8 identifiers	-8.9%	-7.5%	28 / 6

Serial p50, PUT and GET: v1 627 and 621 us, v2 with 8 identifiers 678 and 672 us.

v1 is unchanged. v2 costs 4 to 9% on writes, rising with the identifier count because the walk runs inside the single EVAL rather than in extra round trips. Reads are flat at 6 to 7.5% whatever the count.

Not covered: concurrent writers, so the create-if-absent and alias-healing branches were not exercised under contention.

The harness is in #57, as an optional workflow that gates nothing, in case it is worth having.

[Frenzie]

Alright, looks decent. Pinging @poire-z @hius07 @Commodore64user @mergen3107 for a second opinion on the proposed API.

@pid1 I assume you also intend to open a corresponding PR for the kosync plugin?

[pid1]

Yeah, I figured we'd get the implementation agreed upon here before we did that. I'll make the corresponding PR and link it here once y'all have a chance to agree that this implementation looks good.

[mergen3107]

My usual questions are:

1. What exact problem does this PR solve? Any particular examples (formats) and use cases?
2. Has this been tested on eink devices aside from the laptop?

[Frenzie]

What exact problem does this PR solve? Any particular examples (formats) and use cases?

It seems the main motivation is working around some rather bizarre-sounding behavior by something called CrossInk, that involves recompressing an EPUB. Though I assume it must strip out unused CSS or some such along the way, since just recompressing a zip file sounds rather nonsensical. But setting that aside, I can see why some people might think the current options are a bit restrictive.

Has this been tested on eink devices aside from the laptop?

This is the server side. The only noticeable performance difference on the client side would be if the server became noticeably slower. I.e., it says here this is at least 5% slower which may or may not be a big deal depending on how close to capacity you are.

PS Forgot to ping @NiLuJe.

[mergen3107]

Thanks!
Although I am not sure how to test server-side changes myself :D

[pid1]

Has this been tested on eink devices aside from the laptop?

FWIW, I can build the corresponding plugin-side code for this, test it from a Kindle, and we can ship that all together, if that's preferable.

[pid1]

If the 5% is a concern (given the thousands of ops/second required to expose it), we can land some improvements for Redis (connection pooling would be one of the first) to improve the scalability of the server.

[Frenzie]

Not as such; it's within my expectations given that it has more to do.

[pid1]

I took a crack at some low hanging fruit I found while benchmarking. #58 is pretty simple, just makes us open one redis connection per request instead of two. #59 implements the connection pooling. pool = 5 was already set but unused as implemented.

[Commodore64user]

Can you provide a TL:DR ELI5 explanation of the new API, i understand previously was either a hash comparison or file name comparison.

[pid1]

Can you provide a TL:DR ELI5 explanation of the new API, i understand previously was either a hash comparison or file name comparison.

Yeah, that's correct. Instead of picking one identifier, this allows sending several in priority order. The server tries each in turn and tells you which one matched, and whether the stored position was written by a copy that shares that identifier. A renamed or recompressed copy still finds its place, and a weak match is a signal to seek by percentage rather than trust the xpointer.

[poire-z]

I'm not familiar with/interesting in/ using kosync, so no real opinion.
From the post just above, it's very limited in its purpose/features (actually, no feature, just solving a niche use case - fine with me anyway).
I remember I had a discussion about a kosync API v2 at koreader/koreader#15182 (comment) , which felt more wide (and more interesting to users maybe). I probably discussed it just to save our statistics plugin from being raped :).
But this API version bump to some v2 still feels too major for the described addition.
But again, I don't really care.

[Frenzie]

I'm not familiar with/interesting in/ using kosync

Me neither; I just know Redis and servers (and even syncing to a point) so I can say half sensible things about it.

[pid1]

Fair point, dropped the version bump. It's optional fields on v1 now: identifiers on the PUT, ids on the GET. A client that doesn't send them gets the same response it gets today, and config/routes.lua is untouched.

That also leaves a v2 free for the wider thing in koreader/koreader#15182, which seems like a better use of one.

[pid1]

Two fixes pushed, both from finding a way to lose a reading position.

A match on a weak identifier was registering the caller's stronger digests as aliases to it. Two books a library had tagged alike shared only their weakest identifier, so the second one pushed took over the first one's record and overwrote the position stored there — and correcting the tagging did not separate them again, because the second book's own content digest had been glued to the other record on the way through. An identifier ranked above the one that matched is now left alone, so a wrong guess stays confined to the identifier that made it.

I've also dropped the metadata (title + author) identifier from the client, since it is the only one that can match two genuinely different files and structure already covers the recompression case this is for.

[pid1]

Multi-device syncing is exercised end to end on a Kindle Paperwhite 5 SE. The client side is in koreader/koreader#16110.

[Frenzie]

I've also dropped the metadata (title + author) identifier from the client, since it is the only one that can match two genuinely different files and structure already covers the recompression case this is for.

Could you clarify the reasoning? I find the recompression thing by far the dumbest that hardly needs to be accommodated.

[pid1]

metadata is the only identifier that survives a spine change, like a Calibre conversion. I'd dropped it because two volumes tagged with the series title merge, and the second push overwrites the first's position. progress_match already tells a reader not to trust a weak match. Nothing tells a writer not to clobber on one. I'd like to add that, which makes metadata safe to restore. I'll do that now.

[Frenzie]

Alright.

[pid1]

This should be ready for re-review now. I built out the client-side implementation of this for KOReader, CrossPoint, and Readest, and tested each successfully, including the weird CrossPoint "optimize epub" option.