From d62d6f0111cb0f19fa0a6109f3e26e948b7de0fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juraj=20Or=C5=A1uli=C4=87?= Date: Wed, 17 Jun 2026 23:05:13 +0200 Subject: [PATCH 1/3] Register converter output in the file cache, and detect failed conversions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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) Signed-off-by: Juraj Oršulić --- lib/Document/ConverterBinary.php | 21 +++++++++++++++++-- lib/LocalAppData.php | 35 +++++++++++++++++++++++++++++++- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/lib/Document/ConverterBinary.php b/lib/Document/ConverterBinary.php index 47a44d2..5e55b29 100644 --- a/lib/Document/ConverterBinary.php +++ b/lib/Document/ConverterBinary.php @@ -53,6 +53,14 @@ public function run(string $param, string $password = null): string { } $process = proc_open($cmd, $descriptorSpec, $pipes, self::BINARY_DIRECTORY, ["LD_LIBRARY_PATH" => "."]); + // proc_open returns false if x2t couldn't be spawned at all (missing + // binary, fork failure). Without this the next lines fclose/read null + // pipes and proc_close(false) warns, and the caller silently gets no + // output instead of a clear failure. + if ($process === false) { + throw new DocumentConversionException("failed to start x2t"); + } + @fclose($pipes[0]); $output = @stream_get_contents($pipes[1]); $error = @stream_get_contents($pipes[2]); @@ -65,9 +73,18 @@ public function run(string $param, string $password = null): string { if ($error) { throw new DocumentConversionException($error); - } else { - return $output; } + + // x2t can fail with a nonzero exit status while writing nothing to stderr; + // without this the caller treats a failed conversion as success and later + // trips over the missing Editor.bin, see #70. Stderr is checked first so its + // (more specific) message wins, including the "Empty sFileFrom or sFileTo" + // string that test() relies on. + if ($status !== 0) { + throw new DocumentConversionException("x2t exited with status $status"); + } + + return $output; } public function test(): bool { diff --git a/lib/LocalAppData.php b/lib/LocalAppData.php index be191e9..9a1bd72 100644 --- a/lib/LocalAppData.php +++ b/lib/LocalAppData.php @@ -23,6 +23,7 @@ namespace OCA\DocumentServer; +use OCP\Files\Cache\IScanner; use OCP\Files\Folder; use OCP\Files\File; use OCP\Files\IAppData; @@ -30,6 +31,8 @@ use OCP\Files\SimpleFS\ISimpleFolder; use OCP\IConfig; use OCP\ITempManager; +use OCP\Lock\LockedException; +use Psr\Log\LoggerInterface; /** * Provide local access to appdata folders @@ -38,15 +41,18 @@ class LocalAppData { private $appData; private $config; private $tmpManager; + private LoggerInterface $logger; public function __construct( IAppData $appData, IConfig $config, - ITempManager $tmpManager + ITempManager $tmpManager, + LoggerInterface $logger ) { $this->appData = $appData; $this->config = $config; $this->tmpManager = $tmpManager; + $this->logger = $logger; } /** @@ -100,6 +106,33 @@ public function getReadWriteLocalPath(ISimpleFolder $folder, callable $callback) if (is_dir($localPath)) { $callback($localPath); + // The callback (e.g. x2t producing Editor.bin) writes straight to the + // on-disk appdata folder, bypassing the Nextcloud storage layer, so the + // file cache never learns about the new files and a later getFile() would + // throw NotFoundException. Rescan the folder so the writes become visible, + // see #70 (editor stuck on "Loading document" because Editor.bin is on disk + // but absent from oc_filecache). The temp-folder branch below does not need + // this because copyFromLocal() writes back through File::putContent(), + // which updates the cache itself. The scan must be RECURSIVE: x2t emits + // not just Editor.bin but a media/ subfolder of embedded images, and + // the open flow reads those back through the file API + // (DocumentStore::getEmbeddedFiles -> media/.getDirectoryListing()). A + // shallow scan would register Editor.bin but leave media/'s children + // uncached, so embedded images would silently fail to load. A + // concurrent writer can hold the scan lock; mirror NC core's + // backgroundScan and skip rather than fail the open, the next access + // rescans. + try { + $fullFolder->getStorage()->getScanner()->scan( + $fullFolder->getInternalPath(), + IScanner::SCAN_RECURSIVE + ); + } catch (LockedException $e) { + $this->logger->warning( + 'documentserver: appdata rescan after conversion skipped, folder locked', + ['exception' => $e, 'app' => 'documentserver_community'] + ); + } } else { $localPath = $this->tmpManager->getTemporaryFolder(); $this->copyToLocal($fullFolder, $localPath); From fb5532cfa1749eda1b93b7978002b9cd60ba2d07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juraj=20Or=C5=A1uli=C4=87?= Date: Wed, 17 Jun 2026 23:05:13 +0200 Subject: [PATCH 2/3] Surface a failed open/save to the editor instead of swallowing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Signed-off-by: Juraj Oršulić --- lib/Controller/DocumentController.php | 71 ++++++++++++++++++++++++++- 1 file changed, 70 insertions(+), 1 deletion(-) diff --git a/lib/Controller/DocumentController.php b/lib/Controller/DocumentController.php index e1db263..deaa0f6 100644 --- a/lib/Controller/DocumentController.php +++ b/lib/Controller/DocumentController.php @@ -187,7 +187,47 @@ public function upload(int $docId, string $index) { public function download(int $docId, string $cmd) { $cmd = json_decode($cmd, true); $content = fopen('php://input', 'r'); - $title = $this->documentStore->convertForDownload($docId, $content, $cmd); + + // A failed save-as conversion must be reported to the editor as a 'save' + // error, not escape as an uncaught 500: this endpoint has no other + // handler, and the editor is waiting on a documentOpen 'save' reply. Tell + // the session it failed (-4 = sdkjs DownloadError) so the user sees a + // failed download instead of a silent hang, then return the same error to + // the HTTP caller. The reply push is itself guarded: if the IPC backend is + // what failed, getChannel() would throw and re-raise an uncaught 500. + try { + $title = $this->documentStore->convertForDownload($docId, $content, $cmd); + } catch (\Exception $e) { + $this->logger->warning('documentserver download conversion error: {error}', [ + 'error' => $e->getMessage(), + 'exception' => $e, + ]); + + try { + $session = $this->sessionManager->getSessionForUser($cmd['userconnectionid']); + if ($session) { + $this->ipcFactory->getChannel($session->getSessionId())->pushMessage(json_encode([ + 'type' => 'documentOpen', + 'data' => [ + 'type' => 'save', + 'status' => 'err', + 'data' => -4, + ], + ])); + } + } catch (\Exception $replyError) { + $this->logger->warning('documentserver download could not deliver error reply: {error}', [ + 'error' => $replyError->getMessage(), + 'exception' => $replyError, + ]); + } + + return new DataResponse([ + 'type' => 'save', + 'status' => 'err', + 'data' => -4, + ]); + } $session = $this->sessionManager->getSessionForUser($cmd['userconnectionid']); if ($session) { @@ -361,6 +401,35 @@ private function handleEngineIOPacket(string $packet, string $sid, string $docum 'error' => $e->getMessage(), 'exception' => $e, ]); + + // Don't let a failed command (e.g. the document failing to + // open) disappear into the log: with no reply the editor stays + // on "Loading document" forever. Push a documentOpen error so + // the client surfaces it instead of spinning. Same message shape + // the client already handles for the password case in + // OpenDocument::openDocument; sdkjs parses data as a + // c_oAscError.ID and shows the matching error, -4 is + // DownloadError ("download failed"), the closest generic + // open-failure code. Guard the reply itself: if the original + // failure was the IPC backend being unavailable, getChannel() + // throws too, and an unguarded re-throw here would turn the + // swallowed error back into an uncaught 500. + try { + $sessionChannel = $this->ipcFactory->getChannel($sid); + $sessionChannel->pushMessage(json_encode([ + 'type' => 'documentOpen', + 'data' => [ + 'type' => 'open', + 'status' => 'err', + 'data' => -4, + ], + ])); + } catch (\Exception $replyError) { + $this->logger->warning('documentserver socketIO could not deliver error reply: {error}', [ + 'error' => $replyError->getMessage(), + 'exception' => $replyError, + ]); + } } } // sio types '0' (CONNECT) and '1' (DISCONNECT) require no action here. From cf345753255ecd8836db788702429ef9ec6e6cb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juraj=20Or=C5=A1uli=C4=87?= Date: Wed, 17 Jun 2026 23:05:13 +0200 Subject: [PATCH 3/3] ConvertController: call parent::__construct and return structured conversion errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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": } 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) Signed-off-by: Juraj Oršulić --- lib/Controller/ConvertController.php | 30 +++++++++++++++++++++++++--- 1 file changed, 27 insertions(+), 3 deletions(-) diff --git a/lib/Controller/ConvertController.php b/lib/Controller/ConvertController.php index 010c9cc..68ee2b1 100644 --- a/lib/Controller/ConvertController.php +++ b/lib/Controller/ConvertController.php @@ -23,13 +23,16 @@ namespace OCA\DocumentServer\Controller; +use OCA\DocumentServer\Document\DocumentConversionException; use OCA\DocumentServer\Document\DocumentStore; +use OCA\DocumentServer\Document\PasswordRequiredException; use OCA\DocumentServer\OnlyOffice\URLDecoder; use OCA\DocumentServer\JSONResponse; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; use OCP\AppFramework\Http\Attribute\PublicPage; +use OCP\IRequest; use OCP\IURLGenerator; class ConvertController extends Controller { @@ -37,7 +40,14 @@ class ConvertController extends Controller { private $urlDecoder; private $urlGenerator; - public function __construct(DocumentStore $documentStore, URLDecoder $urlDecoder, IURLGenerator $urlGenerator) { + public function __construct( + string $appName, + IRequest $request, + DocumentStore $documentStore, + URLDecoder $urlDecoder, + IURLGenerator $urlGenerator + ) { + parent::__construct($appName, $request); $this->documentStore = $documentStore; $this->urlDecoder = $urlDecoder; $this->urlGenerator = $urlGenerator; @@ -57,8 +67,22 @@ public function convert(bool $async, string $url, string $outputtype, string $fi } else { $documentId = (int)$key; $documentFile = $this->urlDecoder->getFileForUrl($url); - $this->documentStore->getDocumentForEditor($documentId, $documentFile, $filetype); - $this->documentStore->convert($documentId, $outputtype); + + // A failed conversion must come back as a structured {"error": } + // body, not as an uncaught exception: this is a plain HTTP endpoint + // with no other handler, so without this catch the converter's + // exception escapes as a 500 the caller can't interpret. This endpoint + // answers the connector over the server-side conversion API, whose + // error codes differ from the editor's c_oAscError space used in the + // socket replies: -5 incorrect password, -3 conversion error. + try { + $this->documentStore->getDocumentForEditor($documentId, $documentFile, $filetype); + $this->documentStore->convert($documentId, $outputtype); + } catch (PasswordRequiredException $e) { + return new JSONResponse(['error' => -5]); + } catch (DocumentConversionException $e) { + return new JSONResponse(['error' => -3]); + } $url = $this->urlGenerator->linkToRouteAbsolute( 'documentserver_community.Document.documentFile', [