fix(wopi): harden file saving against failures wrongly seen as conflicts#5771
Open
juliusknorr wants to merge 1 commit into
Open
fix(wopi): harden file saving against failures wrongly seen as conflicts#5771juliusknorr wants to merge 1 commit into
juliusknorr wants to merge 1 commit into
Conversation
Errors raised inside filesystem write hooks (for example a TypeError from the files_versions listener when a node path cannot be resolved) previously escaped putFile()/postFile() uncaught, because the catch blocks only handled \Exception while a TypeError is an \Error. The request then returned an opaque 500 to the WOPI client, which can in turn surface as a spurious document conflict. - Catch \Throwable (not just \Exception) so hook errors produce a controlled response and a meaningful log entry instead of an uncaught error. - Snapshot the mtime before writing and log a distinct error if it changed despite a failed save (a partial write), since an advanced mtime makes the client assume the document changed externally. - Return 409 Conflict instead of 500 for LockedException so the client retries the save instead of treating the lock as a server error. Signed-off-by: Julius Knorr <jus@bitgrid.net> Assisted-by: Claude Code
CarlSchwan
reviewed
Jun 17, 2026
| $this->logger->warning($e->getMessage(), ['exception' => $e]); | ||
| return new JSONResponse([], Http::STATUS_NOT_FOUND); | ||
| } catch (\Exception $e) { | ||
| } catch (\Throwable $e) { |
Member
There was a problem hiding this comment.
not a fan of catching a Throwable, the fix in server is almost fully backported
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Hardens the WOPI save paths so that a failed save can no longer surface to the Collabora client as a spurious document conflict, and so that errors raised inside filesystem write hooks no longer escape as uncaught errors.
Background and full investigation: #5770.
On a production system (NC 34) saves intermittently failed with
500 Internal Server Errorand the client reported a conflict. The root cause of the 500 is aTypeErrorthrown inside thefiles_versionswrite hook (getPathForNode()returnsnull→Storage::store(null)→View::basicOperation(…, null)). That is anextcloud/serverbug already fixed on master in nextcloud/server#60842 (backport to stable34: #61189). Because aTypeErroris an\Errorand not an\Exception, ourcatch (\Exception)did not catch it, so it escaped as an opaque 500 — which the client can then reconcile into a false conflict.Changes
\Throwable(not just\Exception) inputFile()andpostFile(), so errors raised inside filesystem hooks produce a controlled response and a meaningful log entry instead of an uncaught error.LockedException→409 Conflictinstead of500, so the client retries the save instead of treating a transient lock as a server error.This does not (and should not) suppress conflicts caused by a genuinely concurrent successful save — it only ensures a failed save never looks like an external change.
Follow-up (not in this PR)
Helper::toISO8601()second-only precision (always.000000) to reduce timestamp-rounding false positives — needs validation against the Collabora client and is tracked separately.Testing
php -lclean. Manual reasoning against the reported production traces; no automated test added for the hook-error path as it requires the versions listener fault injection.AI tool use disclosure
This change was prepared with the assistance of an AI coding agent (Claude Code). The investigation, code changes, and this description were AI-assisted and reviewed by the committer before submission. Commits carry an
Assisted-by:trailer.https://claude.ai/code/session_0148r9TcHSywDqJVHqyMggub
Generated by Claude Code