[chadek]

What this does

Started as two onlyoffice admin settings that say something is happening and, on this server, are not: the format matrix and the editor's Save button. It then grew a review pass over this branch and over master, and now also carries fourteen fixes found that way — including one that loses a change under simultaneous editing.

Rebased on master — the other two commits this branch used to carry are in already, by way of #396, whose versions are better, so they are gone from here. The chat fix landed separately as #389.

Sixteen commits, each doing one thing.

1. Stop overwriting the admin's format settings on every request

AutoConfig::autoConfigIfNeeded() runs from boot() — so on every request — and re-applied syncSupportedFormats() against a hardcoded list. Anything the admin enabled that the list did not mention was switched back off by the next page load:

admin enables pdf/docm/html editing:   pdf=True  docm=True  html=True
-> loading the files app once:         pdf=False docm=False html=False

The list had also gone stale. It predated 9.x and named twelve editable formats where the bundled server declares twenty-nine — and three of the twelve (doc, ppt, xls) it cannot edit at all, only auto-convert.

Formats are now seeded once, at initial auto-config, from the format matrix the bundled server ships, and never touched again; from there the admin owns them. Seeding still earns its keep, because the connector leaves the lossy-editable formats (odt, ods, odp, csv, rtf, txt) off by default — that was the point of doing it at all. Default openers stay a curated ten rather than following capability, since making OnlyOffice the default handler for .txt or .html would fight Nextcloud's own apps.

A fresh install now seeds 29 editable formats where it seeded 12.

Reading the matrix out of the package is lib/OnlyOffice/BundledFormats.php, whose path is injectable so the unit test does not need the document server to have been built.

2. Make the editor's Save button work

"Keep intermediate versions when editing" sets editorConfig.customization.forcesave, which gives the editor a Save button. Pressing it sends a forceSaveStart socket command the dispatcher had no handler for, so the command was dropped and the button did nothing — the setting looked broken from the settings UI.

The new handler answers with the two-part reply the client expects (forceSaveStart then forceSave). Both carry the same timestamp, because the client stores the one from forceSaveStart and ignores a forceSave whose time does not match it. The save is synchronous, so a failure is reported as an error rather than as a save that starts and never lands, and a press with nothing typed since the last write is answered NotModified — the editor's own state for that — rather than as a save that did not happen.

It snapshots rather than flushes. flushChanges() is written for a session that is ending: it consumes the change list and, when no session is left, closes the document. Driving that from the Save button meant doing it to a document people are still typing into — the change list is the only record of what was typed, since Editor.bin stays at the version the document was opened at, so a session opening next would render the document from before the save and the eventual flush would write a file that had lost the earlier edits.

Also adds documentserver:flush --snapshot, which is that same write on its own: for cron at a chosen interval, or to get every open document on disk on demand.

---

The rest came out of reviewing this branch and the release before it. Each is its own commit.

Losing work

changesIndex was the highest index where sdkjs wants a count

This is the one worth reading. sdkjs keeps this number as its synced position and derives absolute positions from it by adding its own offsets — on the wire deleteIndex += this.changesIndex, and locally Changes.length = SyncIndex + deleteIndex, where a length is plainly a count. We sent getMaxChangeIndexForDocument(), which is count - 1, so every position the client derived came back one too low.

Logging the incoming values on the rig showed the arithmetic exactly, twice over:

deleteIndex=350  maxBefore=266  nchanges=84     350 = 266 + 84
deleteIndex=330  maxBefore=246  nchanges=84     330 = 246 + 84

It hides because a save that carries changes still derives a position past the end of the store, so the delete matches nothing — those two probe runs showed rows before=247, rows after=247 and passed. But sdkjs also sends a saveChanges with an empty change array when all it has to say is a deleteIndex (0 < aChanges.length || null !== deleteIndex), and there the off-by-one is a request to delete one change that should have survived. One change, gone from the file and from the document when it is reopened.

The guard around the delete was wrong too. The key is absent from most saves and -1 when the client explicitly has nothing to discard — sdkjs guards its own arithmetic with -1 !== this.deleteIndex — and both reached deleteChangesByIndex(), because the key was read unguarded and the test was truthiness. -1 is truthy in PHP, so it asked for change_index >= -1: every change in the document. Never observed firing, but it is one client message away.

A document-wide cut is the right semantic, for the record — sdkjs converts to an absolute document index on purpose.

A document was never seen to be empty

getSessionsForDocument() counted every row for the document. Rows are only deleted by cleanSessions(), which runs from the background job, so between two job runs the table holds every session that ever stopped polling — a browser that was killed, a laptop that was closed. Counting those meant the last editor leaving never looked like the last editor leaving: SessionCloser::sessionLeft() did not write the file, and the write fell back to whenever cron next ran, which is exactly the wait #100 exists to remove. It also made expireSession() do nothing observable.

Two notes from getting this wrong on the way:

• expireSession() backdates past the cutoff now instead of writing zero. Zero happens to be far enough in the past against a real clock, but it says nothing about why — and a test with a small clock found it out.
• A departing editor's row is deleted, not expired, and the docblocks now say why, because the two look interchangeable and are not. The poll that session left behind keeps running server-side for up to Channel::TIMEOUT and marks it as seen every Channel::SEEN_INTERVAL, so an expired row revives itself out of its own orphaned request. I tried expiring — every unit test passed, and the rig failed with "B lost the document server when A left".

Saving

• documentserver:flush finishes its sweep. Returning at the first failure meant one document the converter chokes on left every document after it unwritten, silently — the opposite of what somebody running it before a backup wants. Each is attempted now, failures go to stderr as well as the log, and the exit code says whether it went cleanly.
• The Cleanup job honours autosave_interval. It called saveSnapshot() directly, so it ignored the interval — including the 0 documented as turning the periodic write off — and turned one cron pass into a converter run for every open document.
• The Save button is floored, and refused to read-only sessions. Assembling a document is a converter run, and forceSaveStart is the one place a client asks for one directly: the periodic write is floored at autosave_interval, a command is whatever arrives on the socket. A client sending it after every change turns each keystroke into an x2t run. The floor is deliberately not an app setting, because autosave_interval 0 must not silence the button.

Hardening

• Chat messages are capped where they are stored. The handler took whatever string arrived; the editor's own limit is a maxlength on a textarea, which is no limit for anything that is not the editor. Capped at sdkjs c_oAscMaxCellOrCommentLength and cut by characters, because truncating mid-sequence puts invalid UTF-8 in the history file and json_encode() returns false on that — losing the whole backlog rather than one message.
• The close beacon is rate limited, and sent once. Nothing authenticates /session-closed but the session id and it holds a worker for the grace period. pagehide and unload both fired for the same departure, so the server held a worker through the grace period twice.
• Memcache IPC miss counters expire. inc() is the only atomic counter IMemcache offers and it takes no TTL, and cleanupChannel() removed only the read and write pointers — so a counter for a slot never resolved sat in the cache for the life of the process.
• A missing api.js answers 404 instead of a warning and a 200 with nothing in it.

Build and schema

• The document server package is verified before it is unpacked. The build downloads ~600 MB of native code, including the x2t the web server executes, and took whatever arrived. ds_sha256 is pinned and checked first; a pinned hash also catches a release re-uploaded under the same tag, which the digest the API reports alongside the url cannot. The asset url is matched against browser_download_url rather than the first https://...x86_64.rpm anywhere in the response, and the extracted tree is no longer chmod 777 — that shipped a world-writable document server, so any local account could replace the converter.
• The change index's unique constraint is in an order queries can use. (change_index, document_id) led with the column nothing filters on. Measured on the rig with 2001 rows: the old order plans Using where; Using index over all of them, the new one Select tables optimized away.
• The app version moves to 0.3.4. Nextcloud only replays migrations when the declared version increases, and the tag v0.3.3 still carried 0.3.2 inside — so neither the index migration above nor Drop the database indexes that duplicate a wider one #395's would have run on an upgrade.
• Existing installs get the corrected format matrix. Seeding only runs while the connector has no url, so the fix at the top of this PR reaches new installs and nothing else. A repair step re-seeds an install that still carries exactly what the old hardcoded list wrote, and leaves every other install alone — the old code force-wrote the matrix on every request except where the admin disabled something, and that asymmetry is what makes it safe to decide. It resolves AutoConfig from the container behind an isEnabledForUser guard rather than taking it as a constructor argument: with onlyoffice disabled, an eagerly injected one makes occ upgrade die with Class "OCA\Onlyoffice\AppConfig" does not exist.
• BundledFormats is registered rather than autowired — its one constructor argument is a string, and the container resolves builtin-typed parameters by parameter name before falling back to the default.

Testing

phpunit, green in CI: 86 tests, 232 assertions. New files:

SaveChangesCommandTest	changesIndex is a count; -1 and an absent key delete nothing; a later chunk does not re-apply the delete
SessionManagerTest (+)	an expired session is not a participant; polling keeps one; expireSession drops it at once and a poll revives it
SessionCloserTest	a leaving session is dropped, a session that stopped co-authoring is expired; the document is written when the last one leaves and not before
ChangeStoreTest	the returned index is where the batch actually landed, including after another writer got in first
FlushChangesTest	the sweep finishes past a document that will not assemble, and the exit code reports it
CleanupTest	a live document goes through the autosave interval; one that will not write does not stop the sweep
ChatMessageTest	an overlong message is cut to the editor's own limit, and the cut leaves valid UTF-8
ForceSaveTest (+)	the write is floored rather than run on demand; a read-only session cannot ask for one
SaveHandlerTest (+)	a throttled write is refused inside its interval and not disabled by autosave_interval 0

Every one of these was run against the unfixed code first, so they fail for the reason they exist.

tests/rig, all ten green on a Nextcloud 34 rig with the bundled server:

PASS  formats         67s      PASS  twotab         278s
PASS  smoke          221s      PASS  coedit         201s
PASS  flush-live     165s      PASS  coedit-xlsx    143s
PASS  autosave       190s      PASS  forcesave      220s
PASS  leave          292s      PASS  chat           190s

leave is the one that matters here: it was failing intermittently, roughly two runs in six, with one marker missing from the file and from the reopened document. That is the changesIndex off-by-one. It has passed three consecutive times since.

formats_test gained a fourth section that drives two real occ upgrade runs, because repair steps are not part of occ maintenance:repair — an app's installed version being behind info.xml is the only thing that carries them.

Notes for reviewers

• The commits are independent and each lints on its own.
• occ boots this app, so an occ call made while the connector is unconfigured re-runs auto-configuration. That is why formats_test deletes the connector's url last and puts it back afterwards; it is also why clearing the config with occ config:app:delete to test a fresh install does not work the way you would expect.
• The shipped server/Common/config/default.json is not involved in any of this — nothing in lib/ reads it. It belongs to the Node DocService this app replaces, so it can never be the reason a setting appears not to work.
• A failed rig run leaves polls in flight for up to 25s, and Channel::getResponse() re-creates a session whose row it cannot find — so a reset straight after a test that died mid-edit can be followed by rows appearing from nowhere. Wait it out, or restart the app container, before trusting a session count.