Skip to content

Redis-backed save/auth locks and presence for editorData - #44

Draft
moodyjmz wants to merge 31 commits into
mainfrom
feat/redis-editordata-locks-presence
Draft

Redis-backed save/auth locks and presence for editorData#44
moodyjmz wants to merge 31 commits into
mainfrom
feat/redis-editordata-locks-presence

Conversation

@moodyjmz

@moodyjmz moodyjmz commented Sep 8, 2026

Copy link
Copy Markdown
Member

What this is

Replaces editorDataMemory.js's per-process save/auth locks and presence
with Redis-backed equivalents, correctly shared across docservice
replicas. On a multi-replica deployment, the existing memory backend lets
two co-authors on different replicas both get granted the same save lock,
and makes presence (who's editing) invisible cross-replica.

Opt-in via services.CoAuthoring.server.editorDataStorage: "editorDataRedisLocks"; defaults to the existing editorDataMemory
behavior, so this is not a behavior change for anyone who doesn't set it.

Full design writeup: DocService/REDIS_EDITORDATA.md (problem, scope,
architecture, known gaps).

Scope

Covers save/auth locks and presence only. Block locks, messages,
save-state, force-save, and telemetry still delegate straight to
editorDataMemory, unchanged - still single-replica-only for those.

Verification

  • New Jest suite (tests/unit/editorDataRedis{Locks,Presence,Keys}.tests.js)
    against a real Redis via redis-memory-server - no container needed,
    CI-portable. Replaces three ad hoc scripts that existed before this PR.
  • Two independent review rounds (each: a cold read, an adversarial attempt
    to break the specific claims made in code/docs, and a security/perf
    audit), run with no shared context between passes. Confirmed findings
    were fixed with a regression test each, verified to fail against the
    pre-fix code and pass against the fix.
  • The cross-replica claim itself (a joiner on one replica sees an editor
    active on another; a WOPI-level lock survives one co-author disconnecting
    while another stays active elsewhere) was verified manually against two
    real docservice replicas sharing one Redis, behind a real WOPI host. That
    setup is not a committed integration test.

Known gaps (see REDIS_EDITORDATA.md for detail)

  • Owner-token locks, not fencing tokens - open, not evaluated against the
    write path.
  • No committed multi-replica integration test; the cross-replica claim
    above has only been verified manually.
  • No logging anywhere in this layer's Redis error paths - several failure
    modes degrade silently (correctly, per their fail-open/fail-closed
    contracts) rather than incorrectly, but invisibly. Consistent with the
    rest of this codebase's existing logging density in similar spots, not a
    regression - worth its own follow-up pass.
  • One question from manual testing was never resolved either way: whether
    a WOPI-level lock reliably releases once the last real editor
    disconnects.

Marked draft for those reasons, not because the locks/presence logic
itself is in doubt.

AI use disclosure

Written with Claude Code (claude-sonnet-5) - implementation, the two
review rounds, and this description. Every commit carries an
Assisted-by: trailer; all commits reviewed and signed off by me.

moodyjmz and others added 13 commits September 8, 2026 12:14
Replaces editorDataMemory's per-process save/auth locks and presence
with Redis-backed equivalents, correctly shared across replicas.
Opt-in via services.CoAuthoring.server.editorDataStorage.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Real tests via redis-memory-server, replacing three ad hoc POC scripts.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Problem, scope, architecture, and known gaps in
DocService/REDIS_EDITORDATA.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
- lock()/unlock(): buildKey() now runs inside the try, so malformed
  unicode denies the lock instead of throwing
- cleanup(): now catches its own Redis error instead of aborting the
  caller's cleanup chain
- connect(): kicks the lazily-connecting Redis client so healthCheck()
  can actually turn true
- presence writes: native TTL backstop against an orphaned key, and a
  sweep-tracking failure no longer forces a memory-only fallback

Each with a regression test verified against the pre-fix code.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
updatePresence now reports whether there was actually anything to
refresh; the caller re-adds when there wasn't. Closes a permanent
presence-loss gap the native TTL backstop introduced, found by a
second review round against the previous fix.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
No logging anywhere in the Redis error paths in this layer - known
gap, not fixed here.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
CI's ESLint config requires unused caught errors to be _-prefixed.
Six swallowed-error catch blocks used `err` without referencing it.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…-locks-presence

Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>

# Conflicts:
#	DocService/npm-shrinkwrap.json
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
shardIndex hashed on tenant alone, so a single-tenant deployment - the
common case - put every document in the same one of 16 shards, defeating
the sharding entirely. Found by an independent review pass.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
getPresence's fail-open fallback only sees this replica's own local
connections, so a Redis error at the wrong moment could make hasEditors
wrongly read zero while an editor is genuinely active on another replica -
releasing the WOPI lock and wiping the shared save-lock keys under them.
Found by an independent review pass.

The fallback now marks its result as unreliable; hasEditors treats that
as "assume editors present" rather than "confirmed empty."

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
These ran 25-37% comments against 0-3% in the surrounding code, with the
rationale for each review round's fix written inline. Trimmed to the
invariants a maintainer would otherwise break; the reasoning lives in
REDIS_EDITORDATA.md. 200 comment lines to 78, no behaviour change.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Six that described what the line below them already showed - Lua
conditionals, prefix construction, an empty catch. What's left is facts
that live outside the file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
@moodyjmz

moodyjmz commented Sep 9, 2026

Copy link
Copy Markdown
Member Author

Does this close Euro-Office/DocumentServer#267? No — recording why, so it isn't assumed either way.

What that issue asks for

  1. editorDataStorage: "editorDataRedis" should stop crashing the DocService with MODULE_NOT_FOUND on startup.
  2. Its stated root cause: the module exists in source but is excluded from the .deb pkg build.
  3. What the reporter actually wants: co-editing session state that survives a DocService restart — "session data is lost on DocService restart or timeout."

What this PR does

Adds a different backend, editorDataRedisLocks, covering save/auth locks and presence, and registers it in the pkg manifest so it does ship in the .deb. Everything else still delegates to editorDataMemory:

addLocks, addLocksNX, removeLocks, removeAllLocks, getLocks,
addMessage, removeMessages, getMessages,
setSaved, getdelSaved,
setForceSave, getForceSave, checkAndStartForceSave,
checkAndSetForceSave, removeForceSave,
addForceSaveTimerNX, getForceSaveTimer

Against each ask

  1. Not fixed. "editorDataRedis" still fails after this merges — and it can't be fixed by bundling, because that file isn't in the tree to bundle. It's absent from main, from this branch, and from upstream's default branch. This PR adds a new backend name; it doesn't make the old one resolve.
  2. The diagnosis no longer holds. There's nothing to un-exclude from the build. Relatedly, the stack trace on that issue blames a Common/sources/notificationService.js require of editorDataRedis which has since been removed, so the issue is stale as written rather than merely open.
  3. Partially, and not the part they need. The 17 delegated methods above are still in-process memory, so a restart still drops save-state, force-save state, block locks and messages. Their "File version has changed. The page will be refreshed" symptom is a separate matter again: version status is read from the task-result table (taskResult.selectFileStatus.UpdateVersion), not from the editorData store, so no editorDataStorage value governs it.

What someone on that issue would get from this PR: a Redis-backed lock/presence store that is correctly bundled into the .deb, and cross-replica coherence for those two concerns. What they would not get is restart durability, which is the symptom the issue leads with. Different problem, adjacent module name.

Loose end noted separately: DocService/package.json still lists the nonexistent ./sources/editorDataRedis.js in its pkg manifest — deliberately left out of this PR to keep it focused, but it's the artefact that made that issue look like a packaging-exclusion bug.

moodyjmz and others added 13 commits September 10, 2026 08:43
An independent review pass found the marker is honoured by getEditorsCount
alone; the other presence readers still act on an unreliable read.
Threading it through isn't mechanical - the right behaviour differs per
site - so recording it as a known gap rather than half-fixing it here.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Pull the presenceUnknown check into isPresenceUnreliable() so the readers
that need it share one implementation, and log when it fires.

The log level is a parameter: warn from sites that make a decision, debug
from those called on every document operation. No behaviour change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…ence read

A replica-local presence read cannot show recipients on other replicas, so
publish() must not conclude that only the sender is present. Force
needPublish, and skip the "all connections are local" shortcut, whose
count matches trivially against a local-only result and would deliver the
message in-process instead of over pubsub.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…e zero

closeDocument deletes the document's presence keys on the editor path and
the editor-stat key on the pre-stop viewer path, both gated on a presence
read returning nothing. Skip both when the read is unreliable.

Leaked presence keys are bounded: the write and refresh scripts set a
native PEXPIRE on them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
startForceSave scans presence to decide whether the document is encrypted.
A replica-local read cannot see an encrypted editor on another replica, so
the check returns false and the server converts a document it cannot
decrypt. Treat an unreliable read as encrypted, and set res.code to
UnknownError rather than returning NoError with nothing done.

Integrator-visible: the HTTP forcesave command puts that code in
output.error, so a WOPI host sees error 1 where it previously saw 0.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…rded

isUserReconnect, getOriginalParticipantsId and getParticipantMap keep their
current behaviour and only record, at debug, that the read was degraded.
REDIS_EDITORDATA.md gives the reason for each.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…oyment

The image entrypoint emits iooptions.sentinels unconditionally, listing the
plain Redis server as its own sentinel when none is configured. Spreading
that into new Redis() selects sentinel mode on a deployment that has no
sentinel, where the client never connects and the fail-closed locks then
refuse every save.

Move client construction to editorDataRedisClient.js, selected by a new
services.CoAuthoring.redis.mode: auto (default), standalone, sentinel or
cluster. auto excludes that one fabricated shape and still selects a
credible sentinel list; an explicit mode always wins; absent mode, auto
applies, so existing config needs no change.

Also wires Redis.Cluster for the first time, with enableOfflineQueue false
so an unready cluster denies rather than hangs, and the entrypoint's
default db dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
buildKey distinguished the four key kinds by prefix alone, so one
document's keys hashed to different Redis Cluster slots - CROSSSLOT for
the two-key presence scripts and for the two-key DELs in cleanup() and
removePresenceDocument().

Wrap tenant:docId in a hash tag. The prefix stays outside the braces so it
cannot affect the slot, and the sweep's single-key structures are
unchanged. This changes the key format: across a rolling restart the two
cohorts do not share a lock until the old keys expire.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Both stores swallowed their Redis errors, so a deployment that could not
reach Redis refused every save with nothing in the log.

editorDataRedisReport.js logs the transition into failure at error level,
then stays quiet apart from a once-a-minute reminder carrying the
suppressed count, and logs recovery at info. One reporter per concern:
presence and its doc-expiry sweep report separately, because a reporter
that sees interleaved failures and successes re-arms its own throttle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…ting

Adds: what each presence reader does with an unreliable result and the
warn/debug split; the redis.mode table and why auto does not simply trust
iooptions.sentinels; the hash tag and its rolling-restart caveat; why
commandTimeout alone is not fail-closed on a cluster; and the throttled,
per-concern failure reporting.

Records the startForceSave response-code change per calling path, and
replaces the known gaps that these commits closed with the ones that
remain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…ip it ungated

editorDataRedisLocks -> editorDataRedis. This is the config value operators
already set, following upstream documentation, and setting it currently
crash-loops because no such module exists in any open-source tree.

The module ships in the standard documentserver build with no licence
gate. It stays opt-in: the locks fail closed, so selecting Redis without a
reachable Redis would refuse every save behind a healthy healthcheck.

DocService/package.json's pkg manifest has always listed
./sources/editorDataRedis.js, matching no file; that entry now resolves,
and its siblings are listed alongside it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
2.6.12 standalone and sentinel, 3.0 cluster, derived from the commands the
modules issue rather than asserted.

Recorded as a table because one convenient command can move the floor a
long way: GETDEL, the obvious implementation of getdelSaved, would take
the module to 6.2, where a GET-then-DEL inside Lua is atomic on
everything already required.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Supporting one standalone Redis would be less work. Writing down why it
was not chosen, so the question is reopened on the merits: it reintroduces
the single point of failure the module removes, it is a key-format
decision rather than a deferrable one, and a standalone-only client cannot
detect that it is pointed at a cluster - so the symptom is CROSSSLOT at
runtime, which behind fail-closed locks reads as a healthy deployment that
silently will not save.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
moodyjmz and others added 5 commits September 10, 2026 13:01
Several comments referred to a deployment and to a document revision that
a reader coming to this cold has no way to identify. Reworded to state the
facts directly. No behaviour or test-assertion changes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
commandTimeout settles the promise a caller awaits, but the command object
stays on ioredis's offline queue, and the ready handler re-sends every
queued entry on reconnect without checking whether its promise already
settled. So a lockSave that timed out during a blip and was correctly
reported as denied executes for real once Redis returns, taking a lock the
caller has given up on and will never release - a save outage outliving
the blip by the whole lock TTL, indistinguishable in the log from a
legitimate lock. Only the cluster branch disabled that queue.

Disable it on standalone and sentinel too. Two consequences follow:

connect() now awaits readiness, bounded and swallowing errors. With no
offline queue, commands issued before the client is ready are rejected, so
returning while still connecting would deny the first locks of a freshly
started replica. Redis being down at startup still must not stop the
process booting, and does not - it fails closed.

close() falls back to disconnect(). QUIT is itself a command, so on a
client that never reached a server it is now rejected; closing must not
depend on the connection having worked.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
Two ways a deployment could be configured for a cluster and silently get a
standalone client instead - the healthy-looking, not-actually-sharing
failure that cluster support exists to prevent.

An unrecognised mode fell through to standalone. REDIS_MODE=Sentinel, or a
stray space, and the operator sees connection-refused against localhost
and goes looking at Redis rather than at their own config. Unknown modes
now throw and name the valid set.

Only optionsCluster.rootNodes was read, though the same config block ships
iooptionsClusterNodes, and clusterNodes() assumed a {url} shape - a
{host, port} entry threw a bare Invalid URL. Both spellings are accepted,
along with a plain host:port string, and an entry that is neither is named
in the error. iooptionsClusterOptions is passed through to the cluster.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
removePresence and the document-level sweep are the only paths that delete
presence state. getPresence filters expired members out of the read but
leaves them in Redis, and the sweep tracks a document's latest expiry, so
a document with at least one heartbeating connection is never claimed. On
such a document every ungraceful disconnect - a killed replica, a dropped
network - strands a hash field and a sorted-set member permanently, and
since the member id is per-connection, a user on a flaky link strands a
fresh one on each reconnect.

The write and refresh scripts now drop a bounded batch of expired members
as they go. The batch limit matters: an unbounded unpack hits Lua's
C-stack ceiling.

Pruning runs last in both scripts, after the caller's own score has been
pushed into the future. Pruning first would let a refresh of an entry
already past its expiry delete its own hash field and then re-add its
sorted-set member, leaving a member with nothing behind it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
…ages

The floor table said 2.6.12. That holds for the commands the modules
issue, but the orchestrated image always emits iooptions.username, and
ioredis sends the two-argument AUTH whenever a username is present - which
is Redis 6.0 and later. On Redis 5 the server rejects it, ioredis warns
and continues unauthenticated, and every command then fails NOAUTH
against a server that requires a password. Added as its own row, since
6.0 is the number most deployments actually need.

"On an official image" also conflated two images with opposite mechanisms:
the orchestrated one exports NODE_CONFIG and takes EDITOR_DATA_STORAGE,
while the standalone one writes local.json and ignores that variable.

Records two further gaps: claimExpired materialises a whole backlog in one
tick after an extended outage, and getPresence spreads an uncapped result
into hmget.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Signed-off-by: James Manuel <moodyjmz@users.noreply.github.com>
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.

1 participant