Fix editor stuck on "Loading document" when appdata is on local storage#379
Open
ojura wants to merge 3 commits into
Open
Fix editor stuck on "Loading document" when appdata is on local storage#379ojura wants to merge 3 commits into
ojura wants to merge 3 commits into
Conversation
…sions
On local-storage appdata, getReadWriteLocalPath runs the converter in place and
returns without copyFromLocal(), so x2t's output (Editor.bin and the media/
images) lands on disk underneath the storage layer and the file cache never
learns it exists. The next read, getFile('Editor.bin') and the
getDirectoryListing() on media/, goes through the cache rather than the disk and
throws NotFoundException for files that are right there, the editor never gets
its documentOpen reply, and it hangs on "Loading document". Same missing-Editor.bin
NotFoundException as nextcloud#70 (closed, CSV-scoped); it was never CSV-specific, it hits
any format on local-storage appdata.
LocalAppData now rescans the folder after the in-place callback so the cache
picks up what the converter wrote. The scan has to be recursive: the open path
reads the embedded images out of media/ via getDirectoryListing(), and a shallow
scan would register Editor.bin but leave media/'s children invisible. If a
concurrent writer holds the scan lock the LockedException is logged and skipped
rather than failing the open, since the next access rescans anyway. The
temp-folder branch already writes back through File::putContent and needs none of
this.
ConverterBinary::run also stops mistaking a broken conversion for a good one: it
throws if proc_open returns false, and throws on a non-zero x2t exit instead of
only on stderr. Stderr is still checked first so its more specific message wins,
including the "Empty sFileFrom or sFileTo" string that test() depends on.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Juraj Oršulić <juraj.orsulic@fer.hr>
Now that the converter can throw on a failed conversion, the socketIO open path and the download (save-as) path have to report it. Otherwise the exception is swallowed and the editor sits on "Loading document" forever, which is the exact silent failure that made this bug so hard to find. On the open path, DocumentController pushes a documentOpen error (c_oAscError.ID.DownloadError, -4) so the editor shows an error rather than spinning. That reply is itself wrapped in a try/catch: if the IPC backend is the thing that failed, getChannel() throws too, and an unguarded re-throw there would just turn the swallowed error back into a 500 response. The download path does the same, reported as a 'save' error and returning a structured error body instead of an HTTP 500. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Juraj Oršulić <juraj.orsulic@fer.hr>
…version errors
ConvertController extends Controller but never called parent::__construct and took
no IRequest, so $this->request was null. That is a separate pre-existing bug; this
adds both.
convert() also let a converter exception escape as an uncaught HTTP 500. It now
returns a structured {"error": <code>} body instead. The codes come from the server-side
conversion API, which is a different space from the editor's c_oAscError used in
the socket replies: -5 for an incorrect password, -3 for a conversion failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Juraj Oršulić <juraj.orsulic@fer.hr>
a78bcc9 to
d6657cd
Compare
4c723cd to
cf34575
Compare
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
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.
The bug
When the appdata folder is on local storage, opening a document hangs on "Loading document" forever.
The conversion itself is fine: x2t produces a valid
Editor.binand amedia/folder of embedded images. The problem is that nobody tells Nextcloud's file cache they exist.LocalAppData::getReadWriteLocalPathhas two branches, and the local-storage one (is_dir) runs the converter directly against the on-disk path and returns without callingcopyFromLocal(). So the converter writes its output straight to disk, underneath the storage layer, and the file cache never hears about it. The next read,DocumentStore::getFile('Editor.bin')(and thegetDirectoryListing()onmedia/for the embedded images), goes through the file cache rather than stat-ing the disk, so it throwsOCP\Files\NotFoundExceptionfor files that are sitting right there. That exception gets swallowed, the editor never receives itsdocumentOpenreply, and it spins.This is the same missing-
Editor.binNotFoundExceptionreported for CSV in #70 (closed). It was never really about CSV: it hits any document on local-storage appdata, on open, save-as, and convert alike. On object-store appdata the other branch runs, which writes back throughFile::putContentand updates the cache, so none of this applies there.The fix
The core of it is in
LocalAppData::getReadWriteLocalPath: after the in-place callback runs, rescan the folder so the cache learns about what the converter just wrote. The rescan has to be recursive, because the open path reads the embedded images out ofmedia/viagetDirectoryListing(), and a shallow scan would registerEditor.binbut leavemedia/'s children invisible. If a concurrent writer is holding the scan lock, theLockedExceptionis logged and skipped rather than failing the open; the next access will rescan anyway.The rest of the change is about not hiding failures, since the stall was so hard to track down precisely because the failure was silent.
ConverterBinary::runnow throws ifproc_openreturnsfalse, and throws on a non-zero x2t exit instead of only on non-empty stderr, so a broken conversion can never be mistaken for a successful one. (Stderr is still checked first so its more specific message wins, including theEmpty sFileFrom or sFileTostring thattest()depends on.)With the converter now able to throw, the three callers have to report it instead of swallowing it or letting it become an HTTP 500 response. On the socketIO open path,
DocumentControllerpushes adocumentOpenerror so the editor shows something instead of spinning; that reply is itself wrapped, because if the IPC backend is the thing that died,getChannel()throws too and an unguarded re-throw would just turn the swallowed error back into a 500 response. The download (save-as) path does the same with asaveerror.ConvertController::convertreturns a structured{"error": ...}body instead of a 500 response, and while there it also gains theparent::__constructcall it was missing (the controller never called it, so$this->requestwas null, a separate pre-existing bug).One deliberate inconsistency: the error code depends on who is listening. The socket replies go to the editor, which runs
datathroughc_oAscError.ID, so they use-4(DownloadError).ConvertControlleranswers the connector over the server-side conversion API, which is a different code space, so it uses-3(conversion) and-5(password).A note on scope
This is against
master, soConverterBinary::runstill has itsstring $param, string $password = nullsignature with the implicit-nullable PHP 8.4 deprecation. I left that alone to keep the change to one thing; #376 handles it.Testing
Nextcloud 33, PHP 8.4, local appdata. Before the fix a real
.docxhung on "Loading document" with theNotFoundExceptionin the log. After it, a fresh reconversion renders, and the file cache holdsEditor.binandmedia/image*.png(which is the proof the scan needs to be recursive: a shallow one would leave those four images out).php -lpasses on all four files.