From d6551a2ed38cf80d34d23a17093f5828bdf0a591 Mon Sep 17 00:00:00 2001 From: enieuwy <121954036+enieuwy@users.noreply.github.com> Date: Thu, 24 Sep 2026 13:57:38 +0800 Subject: [PATCH 1/5] feat(desktop): report Zotero desktop presence and wait for it without polling `zotio desktop status` reports whether Zotero desktop runs from two signals kept apart: the profile lock (an fcntl write lock on .parentlock on macOS and Linux, probed read-only with F_GETLK; an exclusive, delete-on-close parent.lock handle on Windows, probed with a share-mode open) says the process is up, and one /connector/ping says the connector accepts requests, which imports need. running = lock held or connector answered; connector_reachable = the ping answered; state is ready, starting (process up, connector silent: the start-up window, or a disabled connector) or stopped. It exits 0 whatever it finds. `zotio desktop wait` returns at once when the connector answers, and otherwise sleeps on filesystem notifications for the profile and data directories, probing only after a change settles. Once the lock is seen held it re-checks the connector on a capped backoff (250ms..2s) for up to 2 minutes, because the connector listens seconds after the lock and its start writes no file; after that only events cause probes. --timeout exits 14 (new code: a bounded wait ran out), no discoverable profile exits 9, and --watch-stdin exits when a supervisor's pipe closes. It is mcp:hidden because it blocks, like watch and tail. Profile discovery reuses zoteroprefs: Profiles() exposes the existing profiles.ini discovery (ZOTERO_PROFILE_DIR pin included) and DataDir() mirrors Zotero.DataDirectory.init (dataDir only under useDataDir, else /Zotero, with the Snap/Flatpak home on Linux). prefs.js reading is factored into readPrefs so both paths share the bounded, UTF-8-checked reader. New dependency github.com/fsnotify/fsnotify v1.10.1 (BSD-3-Clause; inotify, kqueue, ReadDirectoryChangesW). Its only dependency, golang.org/x/sys, was already linked. THIRD_PARTY_LICENSES.txt, the command/capability reference and the MCP surface golden are regenerated. --- CHANGELOG.md | 24 ++ README.md | 5 +- SKILL.md | 2 + THIRD_PARTY_LICENSES.txt | 30 ++ dev/zotero-api-coverage.md | 14 + docs/reference/capabilities.md | 2 + docs/reference/commands.md | 111 +++++++ go.mod | 1 + go.sum | 2 + internal/cli/desktop.go | 283 ++++++++++++++++++ internal/cli/desktop_test.go | 227 ++++++++++++++ internal/cli/group_fanout.go | 4 + internal/cli/helpers.go | 6 + internal/cli/root.go | 1 + internal/cli/which.go | 10 + internal/desktop/helper_test.go | 101 +++++++ internal/desktop/lock_other.go | 14 + internal/desktop/lock_unix.go | 59 ++++ internal/desktop/lock_windows.go | 50 ++++ internal/desktop/lockhold_unix_test.go | 26 ++ internal/desktop/lockhold_windows_test.go | 24 ++ internal/desktop/presence.go | 248 +++++++++++++++ internal/desktop/presence_test.go | 178 +++++++++++ internal/desktop/wait.go | 227 ++++++++++++++ internal/desktop/wait_test.go | 273 +++++++++++++++++ .../mcp/testdata/surface_mirror.golden.json | 9 + .../zoteroprefs/presence_discovery_test.go | 100 +++++++ internal/zoteroprefs/zoteroprefs.go | 244 ++++++++++----- 28 files changed, 2206 insertions(+), 69 deletions(-) create mode 100644 internal/cli/desktop.go create mode 100644 internal/cli/desktop_test.go create mode 100644 internal/desktop/helper_test.go create mode 100644 internal/desktop/lock_other.go create mode 100644 internal/desktop/lock_unix.go create mode 100644 internal/desktop/lock_windows.go create mode 100644 internal/desktop/lockhold_unix_test.go create mode 100644 internal/desktop/lockhold_windows_test.go create mode 100644 internal/desktop/presence.go create mode 100644 internal/desktop/presence_test.go create mode 100644 internal/desktop/wait.go create mode 100644 internal/desktop/wait_test.go create mode 100644 internal/zoteroprefs/presence_discovery_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 7eff4fa8..ac613891 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,30 @@ Notable changes to zotio. Format follows [Keep a Changelog](https://keepachangel ## [Unreleased] +### Added + +- **`zotio desktop status` reports whether Zotero desktop is running.** It + reads the profile lock of every discovered Zotero profile (an fcntl lock on + `.parentlock` on macOS and Linux; an exclusive `parent.lock` handle on + Windows) and sends one ping to the local connector. `running` means the + process is up (lock held, or the connector answered); `connector_reachable` + means the connector accepts requests now, which imports need. `state` is + `ready`, `starting` (process up, connector not answering: normal for a few + seconds after launch, and permanent if the connector is disabled) or + `stopped`. It exits 0 whatever it finds. +- **`zotio desktop wait` blocks until Zotero's connector accepts requests.** + It returns at once if the connector already answers. While Zotero is closed + it sleeps on filesystem notifications for the profile and data directories + instead of polling, and probes only after a change; once the lock is held it + re-checks the connector on a capped backoff for up to 2 minutes. `--timeout` + bounds the wait (exit 14, `outcome: "timeout"`); no discoverable profile + exits 9 (`outcome: "no_profile"`); `--watch-stdin` exits when a supervisor's + pipe closes. It is hidden from the MCP surface because it blocks. The + notifications come from the new dependency `github.com/fsnotify/fsnotify` + (BSD-3-Clause; inotify, kqueue, ReadDirectoryChangesW), whose only + dependency, `golang.org/x/sys`, was already linked. +- **Exit code 14: a bounded wait timed out.** Nothing failed; wait again. + ## [0.27.0] — 2026-09-23 ### Changed — breaking diff --git a/README.md b/README.md index ac76d788..cff88dae 100644 --- a/README.md +++ b/README.md @@ -308,7 +308,7 @@ format = "obsidian" # or "logseq" collection:KEY tag:NAME query:TEXT item:KEY saved-search:KEY (needs live desktop) ``` -**Exit codes:** `0` ok · `2` usage · `3` not-found · `4` auth · `5` API · `7` rate-limited · `9` precondition/setup (including *another writer holds the lock* — retry) · `10` config · `11` quality-gate failed · `12` freshness-gate failed · `13` degraded — incomplete: part of a read was unreadable, or part of a batched write was rejected after other elements succeeded. Output is not guaranteed; read the reported failures and reconcile before retrying. +**Exit codes:** `0` ok · `2` usage · `3` not-found · `4` auth · `5` API · `7` rate-limited · `9` precondition/setup (including *another writer holds the lock* — retry) · `10` config · `11` quality-gate failed · `12` freshness-gate failed · `13` degraded — incomplete: part of a read was unreadable, or part of a batched write was rejected after other elements succeeded. Output is not guaranteed; read the reported failures and reconcile before retrying. · `14` timed out — a bounded wait (`desktop wait --timeout`) ended before Zotero's connector answered; wait again. --- @@ -471,6 +471,7 @@ Also available: `--csv`, `--plain`, `--quiet`, `--compact`, and `--deliver stdou ```bash zotio doctor # config, credentials, connectivity, cache freshness, writability +zotio desktop status # is Zotero desktop running, and does its connector accept imports? ``` - **`doctor: connection refused`** — open Zotero desktop and enable **Settings → Advanced → "Allow other applications to communicate with Zotero."** @@ -502,7 +503,7 @@ zotio which "export bibtex for a collection"
Top-level commands -`agent-context` · `analytics` · `annotations` · `attachments` · `auth` · `capabilities` · `collections` · `completion` · `creators` · `demo` · `doctor` · `export` · `feedback` · `groups` · `import` · `init` · `items` · `journal` · `library` · `profile` · `reading-list` · `schema` · `search` · `searches` · `sync` · `tags` · `tail` · `vault` · `version` · `watch` · `which` · `workflow` +`agent-context` · `analytics` · `annotations` · `attachments` · `auth` · `capabilities` · `collections` · `completion` · `creators` · `demo` · `desktop` · `doctor` · `export` · `feedback` · `groups` · `import` · `init` · `items` · `journal` · `library` · `profile` · `reading-list` · `schema` · `search` · `searches` · `sync` · `tags` · `tail` · `vault` · `version` · `watch` · `which` · `workflow`
diff --git a/SKILL.md b/SKILL.md index da087216..e6631e77 100644 --- a/SKILL.md +++ b/SKILL.md @@ -87,6 +87,7 @@ The curated feature set. `zotio which ""` resolves natural-language querie - **`export snapshot`** — Reproducible, resumable full-library JSONL export with a lockfile (key, version, content hash) — diff lockfiles to prove what changed between handoffs, and take one before any bulk write the journal cannot reverse. - **`watch`** — Periodic incremental syncs (`--interval`, `--once`); `--health` diffs library health between cycles and reports new findings to stdout or a webhook. - **`workflow run`** — Run a declarative multi-step spec (JSON) in-process with per-step status and continue-on-error — replaces brittle shell chains. +- **`desktop status` / `desktop wait`** — Is Zotero desktop running, and does its connector accept imports (`connector_reachable`)? `wait` blocks on filesystem events, not a poll, until the connector answers (exit 0), or exits 14 at `--timeout`. - **`init`** — Guided first run (detect Zotero, check the local API and explain how to enable it, set key, first sync, health check); agent-safe under `--no-input` (unmet steps exit 9 with a step report). ### Reading workflow @@ -287,6 +288,7 @@ Explicit flags always win over profile values; profile values win over defaults. | 11 | Quality gate failed (`--fail-on`, `--fail-on-unknown`) | | 12 | Stale data | | 13 | Incomplete — part succeeded, part was rejected; reconcile before retrying | +| 14 | Timed out — `desktop wait --timeout` ended before Zotero's connector answered; wait again | ## Argument Parsing diff --git a/THIRD_PARTY_LICENSES.txt b/THIRD_PARTY_LICENSES.txt index 93c49e2b..42cfd47a 100644 --- a/THIRD_PARTY_LICENSES.txt +++ b/THIRD_PARTY_LICENSES.txt @@ -90,6 +90,36 @@ SOFTWARE. +================================================================================ +github.com/fsnotify/fsnotify v1.10.1 +LICENSE + +Copyright © 2012 The Go Authors. All rights reserved. +Copyright © fsnotify Authors. All rights reserved. + +Redistribution and use in source and binary forms, with or without modification, +are permitted provided that the following conditions are met: + +* Redistributions of source code must retain the above copyright notice, this + list of conditions and the following disclaimer. +* Redistributions in binary form must reproduce the above copyright notice, this + list of conditions and the following disclaimer in the documentation and/or + other materials provided with the distribution. +* Neither the name of Google Inc. nor the names of its contributors may be used + to endorse or promote products derived from this software without specific + prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + ================================================================================ github.com/gofrs/flock v0.13.1 LICENSE diff --git a/dev/zotero-api-coverage.md b/dev/zotero-api-coverage.md index cb85b529..fde6eea2 100644 --- a/dev/zotero-api-coverage.md +++ b/dev/zotero-api-coverage.md @@ -87,6 +87,16 @@ of coverage now. created, leaving a childless item and no reason. A locally scanned PDF has no web source, so callers must fall back to the file's own `file://` URI; `connector.SaveAttachment` rejects an empty one up front so the failure is named. +- **Zotero's process and its connector come up at different times.** Measured + 2026-09-24 against Zotero 7 on macOS: the process takes an fcntl write lock on + `/.parentlock` (F_GETLK from another process names the Zotero PID) + about 3s after launch, creates `zotero.sqlite-wal`/`-shm` in the data directory + about 4s after launch, and `/connector/ping` answers 200 a few seconds later, + only while Zotero runs. Windows builds hold `parent.lock` open with share mode 0 + and delete-on-close instead. Only the connector answering proves an import can + proceed; `desktop status` reports both signals and `desktop wait` sleeps on + filesystem events for the profile and data directories until the connector + answers (`internal/desktop`). - **Schema/type endpoints are global**, served under `/api` directly, NOT under the `/users|groups/` library prefix the configured base URL carries: `/api/itemTypes`, `/api/itemFields`, `/api/itemTypeFields`, @@ -177,6 +187,10 @@ Run this when a new Zotero version ships, or periodically: ## Last reviewed +- **2026-09-24** — against the running Zotero 7 desktop on macOS. Confirmed + that a read-only descriptor's F_GETLK on `.parentlock` reports the Zotero PID + while it runs, recorded the lock/WAL/connector start order under Invariants, + and added `desktop status` and `desktop wait` on those signals. - **2026-08-17** — against the live Zotero 7 desktop connector. Established that `POST /connector/saveAttachment` cannot target an existing library item (session-local ids only; `500` live, `400 SESSION_NOT_FOUND` otherwise) and that diff --git a/docs/reference/capabilities.md b/docs/reference/capabilities.md index dd5ffec2..6df09c37 100644 --- a/docs/reference/capabilities.md +++ b/docs/reference/capabilities.md @@ -31,6 +31,8 @@ The machine-readable registry every command is classified against — read vs. w | `creators audit fix` | write | `web_api` | | `synced_store`, `web_api_key` | | `creators rename` | other | | | | | `demo` | read | | | | +| `desktop status` | read | | | | +| `desktop wait` | read | | | | | `doctor` | introspect | | | | | `export` | other | | | | | `export snapshot` | read | | | | diff --git a/docs/reference/commands.md b/docs/reference/commands.md index 219ed31e..70a17bb3 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -719,6 +719,117 @@ zotio demo [flags] | --- | --- | --- | --- | | `--reset` | `bool` | `false` | Delete and re-seed the demo library (also removes demo.db) | +## `zotio desktop` + +Report whether Zotero desktop is running, or wait for it to start + +``` +zotio desktop +``` + +### `zotio desktop status` + +Report whether Zotero desktop is running and its connector accepts requests + +Report whether Zotero desktop is running and whether its connector accepts +requests. Cheap and local: it reads the profile lock of every discovered +Zotero profile and sends one ping to the local connector. It exits 0 whatever +it finds; read the fields, not the exit code. + +Two signals, reported separately: + + running Zotero's process is up: another process holds the + profile lock (.parentlock via fcntl on macOS and Linux, + parent.lock opened exclusively on Windows), or the + connector answered. + connector_reachable GET /ping answered 200 during this check. + Imports and every other connector write need this. + +state is "ready" when the connector answers, "stopped" when neither signal +holds, and "starting" when the process holds its lock but the connector does +not answer. Zotero takes the lock about 3s after launch and its connector +listens a few seconds later, so "starting" is normal briefly after a launch; +it persists if the connector is disabled (Settings -> Advanced -> "Allow other +applications to communicate with Zotero"), moved to another port, or Zotero +is hung. evidence names the strongest signal: connector, profile_lock, none. + +Profiles are discovered from profiles.ini in the platform's Zotero directory +(ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js. + +``` +zotio desktop status +``` + +Examples: + +```bash +zotio desktop status + zotio desktop status --agent +``` + +### `zotio desktop wait` + +Block until Zotero desktop's connector accepts requests + +Block until Zotero desktop's connector accepts requests, then print the +status that proved it. If the connector already answers, it returns at once. + +While Zotero is closed nothing runs on a timer: the command sleeps on +filesystem notifications for the Zotero profile and data directories and +probes only after a change. Once the profile lock is seen held, the connector +is re-checked on a capped backoff for up to 2 minutes, because it starts +listening a few seconds after the lock and its start writes no file. If Zotero +stays up with the connector silent after that, only filesystem changes cause +further checks. + +Two signals, reported separately: + + running Zotero's process is up: another process holds the + profile lock (.parentlock via fcntl on macOS and Linux, + parent.lock opened exclusively on Windows), or the + connector answered. + connector_reachable GET /ping answered 200 during this check. + Imports and every other connector write need this. + +state is "ready" when the connector answers, "stopped" when neither signal +holds, and "starting" when the process holds its lock but the connector does +not answer. Zotero takes the lock about 3s after launch and its connector +listens a few seconds later, so "starting" is normal briefly after a launch; +it persists if the connector is disabled (Settings -> Advanced -> "Allow other +applications to communicate with Zotero"), moved to another port, or Zotero +is hung. evidence names the strongest signal: connector, profile_lock, none. + +Exit codes and the JSON outcome field: + 0 outcome "ready": the connector answers. + 14 outcome "timeout": --timeout passed first; wait again. + 9 outcome "no_profile" (no Zotero profile found and the connector does not + answer) or "watch_failed" (the directories could not be watched). + 10 the configured base URL is not a local Zotero, so there is no connector + to wait for. + 1 interrupted (SIGINT/SIGTERM) or stdin closed under --watch-stdin; nothing + is printed on stdout. + +--timeout replaces the global request timeout for this command; a connector +ping is always bounded to 3s. + +``` +zotio desktop wait [flags] +``` + +Examples: + +```bash +zotio desktop wait + zotio desktop wait --agent --timeout 6h + # Supervised: exit when the supervisor's pipe closes + zotio desktop wait --agent --watch-stdin --timeout 1h +``` + +| Flag | Type | Default | Description | +| --- | --- | --- | --- | +| `--timeout` | `duration` | `0s` | Give up after this long and exit 14, e.g. 30m or 6h (0 = wait indefinitely) | +| `--watch-stdin` | `bool` | `false` | Exit when stdin reaches end of file (for supervisors that hold a pipe open); input is discarded | + ## `zotio doctor` Check CLI health diff --git a/go.mod b/go.mod index bfe91f5f..4071931f 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( ) require ( + github.com/fsnotify/fsnotify v1.10.1 // BSD-3-Clause; `desktop wait` sleeps on inotify/kqueue/ReadDirectoryChangesW instead of polling github.com/gofrs/flock v0.13.1 golang.org/x/text v0.42.0 ) diff --git a/go.sum b/go.sum index 92008495..c999ad70 100644 --- a/go.sum +++ b/go.sum @@ -5,6 +5,8 @@ github.com/dustin/go-humanize v1.0.1 h1:GzkhY7T5VNhEkwH0PVJgjz+fX1rhBrR7pRT3mDkp github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+mFykh5fBlto= github.com/frankban/quicktest v1.14.6 h1:7Xjx+VpznH+oBnejlPUj8oUpdxnVs4f8XU8WnHkI4W8= github.com/frankban/quicktest v1.14.6/go.mod h1:4ptaffx2x8+WTWXmUCuVU6aPUX1/Mz7zb5vbUoiM6w0= +github.com/fsnotify/fsnotify v1.10.1 h1:b0/UzAf9yR5rhf3RPm9gf3ehBPpf0oZKIjtpKrx59Ho= +github.com/fsnotify/fsnotify v1.10.1/go.mod h1:TLheqan6HD6GBK6PrDWyDPBaEV8LspOxvPSjC+bVfgo= github.com/gofrs/flock v0.13.1 h1:jjREztyBeSKBZYAC+mgc1laB+xsgy4kYMf3FbKF2UBo= github.com/gofrs/flock v0.13.1/go.mod h1:sf4BFiHwnvgxa25DlQoDqXQnwRMEOwqxRq37P6MzzmE= github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8= diff --git a/internal/cli/desktop.go b/internal/cli/desktop.go new file mode 100644 index 00000000..13a80227 --- /dev/null +++ b/internal/cli/desktop.go @@ -0,0 +1,283 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. +// Zotero desktop presence: report whether Zotero is running, and wait for its +// connector to accept requests without polling while it is closed. + +package cli + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + "time" + + "github.com/spf13/cobra" + + "zotio/internal/config" + "zotio/internal/connector" + "zotio/internal/desktop" +) + +// errDesktopWaitTimeout and errDesktopWaitStdinClosed are the context causes +// that tell a --timeout expiry and a closed --watch-stdin pipe apart from an +// interrupt. +var ( + errDesktopWaitTimeout = errors.New("desktop wait timed out") + errDesktopWaitStdinClosed = errors.New("stdin closed") +) + +// Wait outcomes, the `outcome` field of `desktop wait` JSON. +const ( + desktopOutcomeReady = "ready" + desktopOutcomeTimeout = "timeout" + desktopOutcomeNoProfile = "no_profile" + desktopOutcomeWatchFailed = "watch_failed" +) + +// desktopWaitResult is `desktop wait` JSON: the status that ended the wait, +// plus how it ended and after how long. +type desktopWaitResult struct { + desktop.Status + Outcome string `json:"outcome"` + WaitedMS int64 `json:"waited_ms"` +} + +const desktopRunningDefinition = `Two signals, reported separately: + + running Zotero's process is up: another process holds the + profile lock (.parentlock via fcntl on macOS and Linux, + parent.lock opened exclusively on Windows), or the + connector answered. + connector_reachable GET /ping answered 200 during this check. + Imports and every other connector write need this. + +state is "ready" when the connector answers, "stopped" when neither signal +holds, and "starting" when the process holds its lock but the connector does +not answer. Zotero takes the lock about 3s after launch and its connector +listens a few seconds later, so "starting" is normal briefly after a launch; +it persists if the connector is disabled (Settings -> Advanced -> "Allow other +applications to communicate with Zotero"), moved to another port, or Zotero +is hung. evidence names the strongest signal: connector, profile_lock, none.` + +func newDesktopCmd(flags *rootFlags) *cobra.Command { + cmd := &cobra.Command{ + Use: "desktop", + Short: "Report whether Zotero desktop is running, or wait for it to start", + Annotations: map[string]string{"mcp:read-only": "true"}, + } + cmd.AddCommand(newDesktopStatusCmd(flags)) + cmd.AddCommand(newDesktopWaitCmd(flags)) + return cmd +} + +func newDesktopStatusCmd(flags *rootFlags) *cobra.Command { + return &cobra.Command{ + Use: "status", + Short: "Report whether Zotero desktop is running and its connector accepts requests", + Long: `Report whether Zotero desktop is running and whether its connector accepts +requests. Cheap and local: it reads the profile lock of every discovered +Zotero profile and sends one ping to the local connector. It exits 0 whatever +it finds; read the fields, not the exit code. + +` + desktopRunningDefinition + ` + +Profiles are discovered from profiles.ini in the platform's Zotero directory +(ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js.`, + Example: ` zotio desktop status + zotio desktop status --agent`, + Args: cobra.NoArgs, + Annotations: map[string]string{"mcp:read-only": "true", "zotio:preflight": "skip"}, + RunE: func(cmd *cobra.Command, _ []string) error { + prober, err := newDesktopProber(flags) + if err != nil { + return err + } + st := prober.Probe(cmd.Context()) + if flags.asJSON || flags.agent { + return printJSONFiltered(cmd.OutOrStdout(), st, flags) + } + renderDesktopStatus(cmd.OutOrStdout(), st) + return nil + }, + } +} + +func newDesktopWaitCmd(flags *rootFlags) *cobra.Command { + var timeout time.Duration + var watchStdin bool + cmd := &cobra.Command{ + Use: "wait", + Short: "Block until Zotero desktop's connector accepts requests", + Long: `Block until Zotero desktop's connector accepts requests, then print the +status that proved it. If the connector already answers, it returns at once. + +While Zotero is closed nothing runs on a timer: the command sleeps on +filesystem notifications for the Zotero profile and data directories and +probes only after a change. Once the profile lock is seen held, the connector +is re-checked on a capped backoff for up to 2 minutes, because it starts +listening a few seconds after the lock and its start writes no file. If Zotero +stays up with the connector silent after that, only filesystem changes cause +further checks. + +` + desktopRunningDefinition + ` + +Exit codes and the JSON outcome field: + 0 outcome "ready": the connector answers. + 14 outcome "timeout": --timeout passed first; wait again. + 9 outcome "no_profile" (no Zotero profile found and the connector does not + answer) or "watch_failed" (the directories could not be watched). + 10 the configured base URL is not a local Zotero, so there is no connector + to wait for. + 1 interrupted (SIGINT/SIGTERM) or stdin closed under --watch-stdin; nothing + is printed on stdout. + +--timeout replaces the global request timeout for this command; a connector +ping is always bounded to 3s.`, + Example: ` zotio desktop wait + zotio desktop wait --agent --timeout 6h + # Supervised: exit when the supervisor's pipe closes + zotio desktop wait --agent --watch-stdin --timeout 1h`, + Args: cobra.NoArgs, + // mcp:hidden: it blocks for as long as Zotero stays closed, like + // watch and tail, and cannot serve as a request/response MCP tool. + Annotations: map[string]string{"mcp:read-only": "true", "mcp:hidden": "true", "zotio:preflight": "skip"}, + RunE: func(cmd *cobra.Command, _ []string) error { + if timeout < 0 { + return usageErr(fmt.Errorf("--timeout must not be negative, got %s", timeout)) + } + prober, err := newDesktopProber(flags) + if err != nil { + return err + } + if prober.Ping == nil { + return configErr(fmt.Errorf("desktop wait needs a local Zotero base URL: %w", prober.ConnectorErr)) + } + + ctx := cmd.Context() + if timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeoutCause(ctx, timeout, errDesktopWaitTimeout) + defer cancel() + } + if watchStdin { + var cancel context.CancelCauseFunc + ctx, cancel = context.WithCancelCause(ctx) + defer cancel(nil) + stdin := cmd.InOrStdin() + go func() { + _, _ = io.Copy(io.Discard, stdin) + cancel(errDesktopWaitStdinClosed) + }() + } + + jsonOut := flags.asJSON || flags.agent + watchDirs := prober.Install.WatchDirs() + if !jsonOut && isTerminal(cmd.ErrOrStderr()) { + fmt.Fprintf(cmd.ErrOrStderr(), "Waiting for Zotero desktop to start (watching %d Zotero directories)...\n", len(watchDirs)) + } + + start := time.Now() + st, waitErr := desktop.Wait(ctx, desktop.WaitOptions{Prober: prober, WatchDirs: watchDirs}) + result := desktopWaitResult{Status: st, WaitedMS: time.Since(start).Milliseconds()} + + var exitErr error + switch { + case waitErr == nil: + result.Outcome = desktopOutcomeReady + case errors.Is(waitErr, errDesktopWaitTimeout): + result.Outcome = desktopOutcomeTimeout + exitErr = timeoutErr(fmt.Errorf("Zotero desktop's connector did not answer within %s (state %q)", timeout, st.State)) + case errors.Is(waitErr, desktop.ErrNoProfile): + result.Outcome = desktopOutcomeNoProfile + exitErr = preconditionErr(desktopNoProfileError(st)) + case ctx.Err() != nil: + // Interrupted or stdin closed: the caller is going away and + // wants no answer on stdout. + return fmt.Errorf("desktop wait canceled: %w", waitErr) + default: + result.Outcome = desktopOutcomeWatchFailed + exitErr = preconditionErr(fmt.Errorf("cannot watch the Zotero directories for a start: %w", waitErr)) + } + + if jsonOut { + if err := printJSONFiltered(cmd.OutOrStdout(), result, flags); err != nil { + return err + } + return exitErr + } + if exitErr != nil { + return exitErr + } + fmt.Fprintf(cmd.OutOrStdout(), "Zotero desktop is ready: the connector answers at %s (waited %s).\n", + st.ConnectorURL, (time.Duration(result.WaitedMS) * time.Millisecond).Round(100*time.Millisecond)) + return nil + }, + } + cmd.Flags().DurationVar(&timeout, "timeout", 0, "Give up after this long and exit 14, e.g. 30m or 6h (0 = wait indefinitely)") + cmd.Flags().BoolVar(&watchStdin, "watch-stdin", false, "Exit when stdin reaches end of file (for supervisors that hold a pipe open); input is discarded") + return cmd +} + +// newDesktopProber discovers the installation and resolves the connector +// from the configured base URL, the same resolution `import` uses. A base URL +// that is not a local Zotero leaves no connector to ping; that is reported in +// the status rather than failing it, because the profile lock still answers +// whether Zotero runs. +func newDesktopProber(flags *rootFlags) (*desktop.Prober, error) { + cfg, err := config.Load(flags.configPath) + if err != nil { + return nil, configErr(err) + } + prober := &desktop.Prober{Install: desktop.Discover(), PingTimeout: desktop.DefaultPingTimeout} + base, ok := connectorBaseFromAPIBase(cfg.BaseURL) + if !ok { + prober.ConnectorErr = fmt.Errorf("the desktop connector is only available with a local Zotero base URL") + return prober, nil + } + conn := connector.New(base, desktop.DefaultPingTimeout) + prober.ConnectorURL = base + prober.Ping = func(ctx context.Context) error { return connectorPing(ctx, conn) } + return prober, nil +} + +func desktopNoProfileError(st desktop.Status) error { + msg := "no Zotero desktop profile directory was found and the connector does not answer; install and start Zotero once, or set ZOTERO_PROFILE_DIR" + if st.DiscoveryError != "" { + msg += " (discovery: " + st.DiscoveryError + ")" + } + return errors.New(msg) +} + +func renderDesktopStatus(w io.Writer, st desktop.Status) { + switch st.State { + case desktop.StateReady: + fmt.Fprintf(w, "Zotero desktop: ready (the connector answers at %s)\n", st.ConnectorURL) + case desktop.StateStarting: + fmt.Fprintln(w, "Zotero desktop: starting (the process holds its profile lock; the connector does not answer yet)") + default: + fmt.Fprintln(w, "Zotero desktop: stopped") + } + if st.ConnectorError != "" { + fmt.Fprintf(w, "Connector: %s\n", SanitizeForTerminal(st.ConnectorError)) + } + for _, p := range st.Profiles { + lock := string(p.Lock) + switch { + case p.Lock == desktop.LockHeld && p.LockPID > 0: + lock = fmt.Sprintf("held by pid %d", p.LockPID) + case p.LockError != "": + lock = "error: " + p.LockError + } + fmt.Fprintf(w, "Profile: %s (lock %s)\n", SanitizeForTerminal(p.Path), SanitizeForTerminal(lock)) + } + if len(st.Profiles) == 0 { + fmt.Fprintln(w, "Profile: none found") + } + if st.DataDir != "" { + fmt.Fprintf(w, "Data directory: %s\n", SanitizeForTerminal(st.DataDir)) + } + if st.DiscoveryError != "" { + fmt.Fprintf(w, "Discovery: %s\n", SanitizeForTerminal(strings.TrimSpace(st.DiscoveryError))) + } +} diff --git a/internal/cli/desktop_test.go b/internal/cli/desktop_test.go new file mode 100644 index 00000000..ab5f5782 --- /dev/null +++ b/internal/cli/desktop_test.go @@ -0,0 +1,227 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +package cli + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" + "time" + + "zotio/internal/connector" + "zotio/internal/zoteroprefs" +) + +// desktopTestEnv isolates discovery and config: profiles come only from the +// returned pinned directory (or from nowhere when pin is false), and the +// connector answers as up says. Nothing here can reach a real Zotero. +func desktopTestEnv(t *testing.T, pin bool, up bool) (flags *rootFlags, profile string) { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + t.Setenv("APPDATA", filepath.Join(home, "AppData")) + t.Setenv("ZOTERO_BASE_URL", "") + t.Setenv("ZOTERO_CONFIG", "") + t.Setenv(zoteroprefs.ProfileDirEnv, "") + if pin { + profile = t.TempDir() + t.Setenv(zoteroprefs.ProfileDirEnv, profile) + } + oldPing := connectorPing + t.Cleanup(func() { connectorPing = oldPing }) + connectorPing = func(context.Context, *connector.Client) error { + if up { + return nil + } + return errors.New("dial tcp 127.0.0.1:23119: connect: connection refused") + } + return &rootFlags{asJSON: true, agent: true, configPath: testConfigFile(t, "http://localhost:23119/api/users/0")}, profile +} + +func runDesktopCmd(t *testing.T, flags *rootFlags, stdin string, args ...string) (map[string]any, string, error) { + t.Helper() + cmd := newDesktopCmd(flags) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&bytes.Buffer{}) + cmd.SetIn(strings.NewReader(stdin)) + cmd.SetArgs(args) + cmd.SilenceErrors, cmd.SilenceUsage = true, true + err := cmd.ExecuteContext(t.Context()) + if out.Len() == 0 { + return nil, "", err + } + var got map[string]any + if jerr := json.Unmarshal(out.Bytes(), &got); jerr != nil { + t.Fatalf("stdout is not one JSON object: %v\n%s", jerr, out.String()) + } + return got, out.String(), err +} + +func desktopJSONKeys(m map[string]any) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + slices.Sort(keys) + return keys +} + +// papio parses this object; its field names and meanings are the contract. +func TestDesktopStatusAgentJSONContract(t *testing.T) { + t.Run("stopped", func(t *testing.T) { + flags, profile := desktopTestEnv(t, true, false) + got, _, err := runDesktopCmd(t, flags, "", "status") + if err != nil { + t.Fatalf("desktop status: %v", err) + } + want := []string{"checked_at", "connector_error", "connector_reachable", "connector_url", "data_dir", "evidence", "profiles", "running", "state"} + if keys := desktopJSONKeys(got); !slices.Equal(keys, want) { + t.Fatalf("status keys = %v, want %v", keys, want) + } + if got["running"] != false || got["connector_reachable"] != false || got["state"] != "stopped" || got["evidence"] != "none" { + t.Fatalf("status = %v, want stopped", got) + } + profiles, _ := got["profiles"].([]any) + if len(profiles) != 1 || profiles[0].(map[string]any)["path"] != profile || profiles[0].(map[string]any)["lock"] != "absent" { + t.Fatalf("profiles = %v, want the pinned profile with an absent lock", got["profiles"]) + } + if got["connector_url"] != "http://localhost:23119/connector" { + t.Fatalf("connector_url = %v", got["connector_url"]) + } + }) + + t.Run("ready", func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, true) + got, _, err := runDesktopCmd(t, flags, "", "status") + if err != nil { + t.Fatalf("desktop status: %v", err) + } + if got["running"] != true || got["connector_reachable"] != true || got["state"] != "ready" || got["evidence"] != "connector" { + t.Fatalf("status = %v, want ready on connector evidence", got) + } + if _, ok := got["connector_error"]; ok { + t.Fatalf("status = %v, want no connector_error when reachable", got) + } + }) +} + +// A Web-API-only configuration has no connector, but the lock still says +// whether Zotero runs, so status reports rather than fails. +func TestDesktopStatusWithANonLocalBaseURLStillReports(t *testing.T) { + flags, _ := desktopTestEnv(t, true, true) + flags.configPath = testConfigFile(t, "https://api.zotero.org/users/123") + got, _, err := runDesktopCmd(t, flags, "", "status") + if err != nil { + t.Fatalf("desktop status: %v", err) + } + if got["connector_reachable"] != false || !strings.Contains(got["connector_error"].(string), "local Zotero base URL") { + t.Fatalf("status = %v, want the missing connector explained", got) + } +} + +func TestDesktopWaitExitCodesAndOutcomes(t *testing.T) { + t.Run("already ready", func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, true) + got, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "1h0m0s") + if err != nil { + t.Fatalf("desktop wait: %v", err) + } + if got["outcome"] != "ready" || got["connector_reachable"] != true || got["running"] != true { + t.Fatalf("wait = %v, want outcome ready", got) + } + if _, ok := got["waited_ms"].(float64); !ok { + t.Fatalf("wait = %v, want a numeric waited_ms", got) + } + }) + + t.Run("timeout exits 14", func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, false) + start := time.Now() + got, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "200ms") + if code := ExitCode(err); code != 14 { + t.Fatalf("exit code = %d (err %v), want 14", code, err) + } + if got["outcome"] != "timeout" || got["state"] != "stopped" { + t.Fatalf("wait = %v, want outcome timeout in state stopped", got) + } + if elapsed := time.Since(start); elapsed > 5*time.Second { + t.Fatalf("a 200ms timeout took %v", elapsed) + } + }) + + t.Run("no profile exits 9", func(t *testing.T) { + flags, _ := desktopTestEnv(t, false, false) + got, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "5s") + if code := ExitCode(err); code != 9 { + t.Fatalf("exit code = %d (err %v), want 9", code, err) + } + if got["outcome"] != "no_profile" { + t.Fatalf("wait = %v, want outcome no_profile", got) + } + }) + + t.Run("non-local base URL exits 10", func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, false) + flags.configPath = testConfigFile(t, "https://api.zotero.org/users/123") + _, out, err := runDesktopCmd(t, flags, "", "wait") + if code := ExitCode(err); code != 10 || out != "" { + t.Fatalf("exit code = %d, stdout %q (err %v); want 10 and no output", code, out, err) + } + }) + + // A supervisor that dies closes its end of the pipe; the waiter must not + // outlive it, and must not print an answer nobody reads. + t.Run("stdin closed under --watch-stdin", func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, false) + _, out, err := runDesktopCmd(t, flags, "", "wait", "--watch-stdin") + if err == nil || ExitCode(err) != 1 || out != "" { + t.Fatalf("exit code = %d, stdout %q (err %v); want 1 and no output", ExitCode(err), out, err) + } + }) + + t.Run("negative timeout is a usage error", func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, false) + _, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "-1s") + if code := ExitCode(err); code != 2 { + t.Fatalf("exit code = %d (err %v), want 2", code, err) + } + }) +} + +// Without --watch-stdin an empty stdin (Go's exec default is /dev/null) must +// not end the wait. +func TestDesktopWaitIgnoresStdinUnlessAsked(t *testing.T) { + flags, _ := desktopTestEnv(t, true, false) + got, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "300ms") + if ExitCode(err) != 14 || got["outcome"] != "timeout" { + t.Fatalf("wait with an empty stdin = %v, exit %d; want it to run to the timeout", got, ExitCode(err)) + } +} + +func TestDesktopStatusHumanOutputNamesTheState(t *testing.T) { + flags, profile := desktopTestEnv(t, true, false) + flags.asJSON, flags.agent = false, false + cmd := newDesktopCmd(flags) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetArgs([]string{"status"}) + if err := cmd.ExecuteContext(t.Context()); err != nil { + t.Fatalf("desktop status: %v", err) + } + for _, want := range []string{"Zotero desktop: stopped", "Profile: " + profile + " (lock absent)", "connection refused"} { + if !strings.Contains(out.String(), want) { + t.Fatalf("output lacks %q:\n%s", want, out.String()) + } + } + if _, err := os.Stat(filepath.Join(profile, ".parentlock")); err == nil { + t.Fatal("status created a lock file in the profile") + } +} diff --git a/internal/cli/group_fanout.go b/internal/cli/group_fanout.go index 2e82dda9..a3aa78db 100644 --- a/internal/cli/group_fanout.go +++ b/internal/cli/group_fanout.go @@ -493,6 +493,10 @@ var fanoutRefusalReasons = map[string]fanoutRefusalReason{ "it is account-level: it reads the account's group membership, not a library"}, "doctor": {fanoutLibraryScoped, "it reports installation, auth and desktop state rather than anything belonging to one library"}, + "desktop status": {fanoutLibraryScoped, + "it reports whether the Zotero desktop process runs, which has no library dimension"}, + "desktop wait": {fanoutFinite, + "it blocks until Zotero desktop starts, and the desktop has no library dimension"}, "which": {fanoutLibraryScoped, "it reads the capability index, which has no library dimension"}, "version": {fanoutLibraryScoped, "it prints the binary version, which has no library dimension"}, "creators audit": {fanoutSideEffectFree, diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index dd99827b..7712072e 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -240,6 +240,12 @@ func freshnessErr(err error) error { return &cliError{code: 12, err: err} } // output), a quality gate (11), a precondition (9), and freshness (12). func degradedErr(err error) error { return &cliError{code: 13, err: err} } +// a bounded wait (`desktop wait --timeout`) ran out before the awaited +// condition held. Nothing failed and nothing needs fixing: the remedy is to +// wait again, unlike a precondition (9), whose remedy is to change the +// environment first. +func timeoutErr(err error) error { return &cliError{code: 14, err: err} } + // dryRunOK reports whether the command should short-circuit without doing any // real work because --dry-run was set. The verify pipeline probes hand-written // commands with --dry-run; commands that put validation in cobra's `Args:` or diff --git a/internal/cli/root.go b/internal/cli/root.go index 9e048a90..417748e1 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -526,6 +526,7 @@ See README.md or the bundled SKILL.md for recipes.`, rootCmd.AddCommand(newTagsCmd(flags)) rootCmd.AddCommand(newCreatorsCmd(flags)) rootCmd.AddCommand(newDoctorCmd(flags)) + rootCmd.AddCommand(newDesktopCmd(flags)) rootCmd.AddCommand(newInitCmd(flags)) rootCmd.AddCommand(newDemoCmd(flags)) rootCmd.AddCommand(newAuthCmd(flags)) diff --git a/internal/cli/which.go b/internal/cli/which.go index b737fb1b..85be3a93 100644 --- a/internal/cli/which.go +++ b/internal/cli/which.go @@ -103,6 +103,16 @@ var whichAliases = map[string][]string{ "tag item into collection", "organise item into collection", }, + "desktop status": { + "is zotero running", + "is zotero desktop open", + "check whether zotero is running", + }, + "desktop wait": { + "wait for zotero to start", + "wait until zotero is open", + "wait for zotero desktop", + }, } // whichSkip lists command names invisible to the which index: meta-commands diff --git a/internal/desktop/helper_test.go b/internal/desktop/helper_test.go new file mode 100644 index 00000000..4be96c95 --- /dev/null +++ b/internal/desktop/helper_test.go @@ -0,0 +1,101 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +package desktop + +import ( + "bufio" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + "time" +) + +// lockHolderEnv turns the test binary into a lock-holding child. The lock +// must live in ANOTHER process: fcntl F_GETLK never reports a lock the +// querying process holds itself, so an in-process holder would prove nothing. +const lockHolderEnv = "ZOTIO_TEST_DESKTOP_LOCK_HOLDER" + +func TestMain(m *testing.M) { + if path := os.Getenv(lockHolderEnv); path != "" { + os.Exit(runLockHolder(path)) + } + os.Exit(m.Run()) +} + +// runLockHolder takes the profile lock the way Zotero does, reports it, and +// holds it until its stdin closes. +func runLockHolder(path string) int { + release, err := holdLock(path) + if err != nil { + fmt.Fprintln(os.Stderr, err) + return 1 + } + fmt.Println("locked") + _, _ = io.Copy(io.Discard, os.Stdin) + release() + return 0 +} + +// lockHolder is a child process holding a profile lock. +type lockHolder struct { + pid int + stdin io.WriteCloser + cmd *exec.Cmd + done bool +} + +// startLockHolder starts a child that holds the Zotero lock file in +// profileDir, and returns once the lock is held. +func startLockHolder(t *testing.T, profileDir string) *lockHolder { + t.Helper() + cmd := exec.Command(os.Args[0], "-test.run=^$") + cmd.Env = append(os.Environ(), lockHolderEnv+"="+filepath.Join(profileDir, lockFileName)) + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatal(err) + } + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatal(err) + } + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + h := &lockHolder{pid: cmd.Process.Pid, stdin: stdin, cmd: cmd} + t.Cleanup(func() { + if !h.done { + _ = cmd.Process.Kill() + _ = cmd.Wait() + } + }) + ready := make(chan string, 1) + go func() { + line, _ := bufio.NewReader(stdout).ReadString('\n') + ready <- strings.TrimSpace(line) + }() + select { + case line := <-ready: + if line != "locked" { + t.Fatalf("lock holder said %q, stderr: %s", line, stderr.String()) + } + case <-time.After(10 * time.Second): + t.Fatalf("lock holder did not take the lock; stderr: %s", stderr.String()) + } + return h +} + +// release makes the child drop the lock and exit, as Zotero does on quit. +func (h *lockHolder) release(t *testing.T) { + t.Helper() + _ = h.stdin.Close() + if err := h.cmd.Wait(); err != nil { + t.Fatalf("lock holder exit: %v", err) + } + h.done = true +} diff --git a/internal/desktop/lock_other.go b/internal/desktop/lock_other.go new file mode 100644 index 00000000..0446bbc6 --- /dev/null +++ b/internal/desktop/lock_other.go @@ -0,0 +1,14 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +//go:build !(darwin || dragonfly || freebsd || linux || netbsd || openbsd || windows) + +package desktop + +import ( + "fmt" + "runtime" +) + +func probeLock(string) LockProbe { + return LockProbe{State: LockError, Err: fmt.Errorf("profile lock probing is not supported on %s", runtime.GOOS)} +} diff --git a/internal/desktop/lock_unix.go b/internal/desktop/lock_unix.go new file mode 100644 index 00000000..accab50e --- /dev/null +++ b/internal/desktop/lock_unix.go @@ -0,0 +1,59 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +//go:build darwin || dragonfly || freebsd || linux || netbsd || openbsd + +package desktop + +import ( + "errors" + "fmt" + "io" + "os" + "path/filepath" + "syscall" +) + +// lockFileName is the file Mozilla's nsProfileLock locks with +// fcntl(F_SETLK, F_WRLCK) over its whole length on macOS and Linux. Zotero +// never deletes it, so only the lock, not the file, says whether Zotero runs. +const lockFileName = ".parentlock" + +func probeLock(profileDir string) LockProbe { + path := filepath.Join(profileDir, lockFileName) + info, err := os.Lstat(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return LockProbe{State: LockAbsent} + } + return LockProbe{State: LockError, Err: err} + } + if !info.Mode().IsRegular() { + return LockProbe{State: LockError, Err: fmt.Errorf("%s is not a regular file (%s)", path, info.Mode().Type())} + } + // Read-only: F_GETLK only asks, and a probe must never create, truncate + // or lock the file Zotero owns. O_NONBLOCK and O_NOFOLLOW keep a file + // swapped for a FIFO or a symlink after the Lstat from blocking open(2) + // or redirecting it. + fd, err := syscall.Open(path, syscall.O_RDONLY|syscall.O_CLOEXEC|syscall.O_NONBLOCK|syscall.O_NOFOLLOW, 0) + if err != nil { + if errors.Is(err, syscall.ENOENT) { + return LockProbe{State: LockAbsent} + } + return LockProbe{State: LockError, Err: fmt.Errorf("opening %s: %w", path, err)} + } + // Closing any descriptor drops every fcntl lock THIS process holds on the + // file. This process holds none, so the close cannot release Zotero's. + defer syscall.Close(fd) + + // F_GETLK reports a conflicting lock held by ANOTHER process; a lock this + // process held would read as unlocked, which is why tests hold it from a + // child process. + lk := syscall.Flock_t{Type: syscall.F_WRLCK, Whence: io.SeekStart} + if err := syscall.FcntlFlock(uintptr(fd), syscall.F_GETLK, &lk); err != nil { + return LockProbe{State: LockError, Err: fmt.Errorf("querying the lock on %s: %w", path, err)} + } + if lk.Type == syscall.F_UNLCK { + return LockProbe{State: LockFree} + } + return LockProbe{State: LockHeld, PID: int(lk.Pid)} +} diff --git a/internal/desktop/lock_windows.go b/internal/desktop/lock_windows.go new file mode 100644 index 00000000..0a87026b --- /dev/null +++ b/internal/desktop/lock_windows.go @@ -0,0 +1,50 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +//go:build windows + +package desktop + +import ( + "errors" + "fmt" + "path/filepath" + "syscall" +) + +// lockFileName is the file Mozilla's nsProfileLock opens on Windows with +// share mode 0 and FILE_FLAG_DELETE_ON_CLOSE: nobody else can open it while +// Zotero runs, and it disappears when the last handle closes, crash included. +const lockFileName = "parent.lock" + +// errorSharingViolation is ERROR_SHARING_VIOLATION: another handle's share +// mode refuses this open. +const errorSharingViolation syscall.Errno = 32 + +func probeLock(profileDir string) LockProbe { + path := filepath.Join(profileDir, lockFileName) + name, err := syscall.UTF16PtrFromString(path) + if err != nil { + return LockProbe{State: LockError, Err: err} + } + // GENERIC_READ, not zero access: an open requesting no data access is not + // checked against share modes and would succeed even while Zotero holds + // the file exclusively. OPEN_EXISTING so the probe never creates it. + h, err := syscall.CreateFile(name, syscall.GENERIC_READ, + syscall.FILE_SHARE_READ|syscall.FILE_SHARE_WRITE|syscall.FILE_SHARE_DELETE, + nil, syscall.OPEN_EXISTING, syscall.FILE_ATTRIBUTE_NORMAL, 0) + if err != nil { + switch { + case errors.Is(err, syscall.ERROR_FILE_NOT_FOUND), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND): + return LockProbe{State: LockAbsent} + case errors.Is(err, errorSharingViolation): + return LockProbe{State: LockHeld} + default: + return LockProbe{State: LockError, Err: fmt.Errorf("opening %s: %w", path, err)} + } + } + // A leftover file nobody holds (possible only if the delete-on-close was + // lost, e.g. to power loss). The handle is closed at once: while it is + // open, a Zotero starting at that instant would fail its exclusive open. + _ = syscall.CloseHandle(h) + return LockProbe{State: LockFree} +} diff --git a/internal/desktop/lockhold_unix_test.go b/internal/desktop/lockhold_unix_test.go new file mode 100644 index 00000000..34623809 --- /dev/null +++ b/internal/desktop/lockhold_unix_test.go @@ -0,0 +1,26 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +//go:build darwin || dragonfly || freebsd || linux || netbsd || openbsd + +package desktop + +import ( + "io" + "syscall" +) + +// holdLock takes the lock as Mozilla's nsProfileLock::LockWithFcntl does: +// open (creating and truncating), then F_SETLK a write lock over the whole +// file. +func holdLock(path string) (func(), error) { + fd, err := syscall.Open(path, syscall.O_WRONLY|syscall.O_CREAT|syscall.O_TRUNC|syscall.O_CLOEXEC, 0o600) + if err != nil { + return nil, err + } + lk := syscall.Flock_t{Type: syscall.F_WRLCK, Whence: io.SeekStart} + if err := syscall.FcntlFlock(uintptr(fd), syscall.F_SETLK, &lk); err != nil { + _ = syscall.Close(fd) + return nil, err + } + return func() { _ = syscall.Close(fd) }, nil +} diff --git a/internal/desktop/lockhold_windows_test.go b/internal/desktop/lockhold_windows_test.go new file mode 100644 index 00000000..b4c7e310 --- /dev/null +++ b/internal/desktop/lockhold_windows_test.go @@ -0,0 +1,24 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +//go:build windows + +package desktop + +import "syscall" + +// fileFlagDeleteOnClose is FILE_FLAG_DELETE_ON_CLOSE. +const fileFlagDeleteOnClose = 0x04000000 + +// holdLock takes the lock as Mozilla's nsProfileLock does on Windows: an +// exclusive (share mode 0) handle that deletes the file when it closes. +func holdLock(path string) (func(), error) { + name, err := syscall.UTF16PtrFromString(path) + if err != nil { + return nil, err + } + h, err := syscall.CreateFile(name, syscall.GENERIC_READ, 0, nil, syscall.OPEN_ALWAYS, fileFlagDeleteOnClose, 0) + if err != nil { + return nil, err + } + return func() { _ = syscall.CloseHandle(h) }, nil +} diff --git a/internal/desktop/presence.go b/internal/desktop/presence.go new file mode 100644 index 00000000..5728cc1e --- /dev/null +++ b/internal/desktop/presence.go @@ -0,0 +1,248 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +// Package desktop reports whether Zotero desktop is running and waits for it +// to start, without polling while it is closed. +// +// # What "running" means +// +// Two independent signals exist, and they answer different questions: +// +// - The profile lock. Zotero is a Mozilla-platform application and holds a +// lock in its profile directory for as long as the process lives: an +// fcntl write lock on ".parentlock" on macOS and Linux, and an exclusive +// (share mode 0, delete-on-close) handle on "parent.lock" on Windows. A +// held lock says the PROCESS is up. It says nothing about whether Zotero +// can accept work yet. +// - The connector. Zotero's HTTP server answers GET /connector/ping with +// 200 once it listens. Imports, saves and every other connector write +// need this, and nothing else proves it. +// +// Status.Running is true when either signal holds. Status.ConnectorReachable +// is true only when the connector answered during this check, and it is the +// field a caller that wants to import must gate on. +// +// # The transition window +// +// Measured against Zotero 7 on macOS, Zotero takes the profile lock about 3s +// after launch and creates its database WAL files about 4s after launch; the +// connector starts listening a few seconds later still. In between, the +// process is up and the connector refuses connections: Status.State is +// "starting". The same state persists if the connector is disabled, bound to +// another port, or Zotero is hung, so "starting" means "process up, connector +// not answering", not a promise that it will answer. +package desktop + +import ( + "context" + "errors" + "fmt" + "slices" + "time" + + "zotio/internal/zoteroprefs" +) + +// LockState is what a profile lock probe observed. +type LockState string + +const ( + // LockHeld means another process holds the profile lock: Zotero runs. + LockHeld LockState = "held" + // LockFree means the lock file exists and nobody holds it. + LockFree LockState = "free" + // LockAbsent means the lock file does not exist. On Windows Zotero deletes + // it on exit; on macOS and Linux it means Zotero never ran on the profile. + LockAbsent LockState = "absent" + // LockError means the probe could not decide. + LockError LockState = "error" +) + +// LockProbe is the result of probing one profile directory's lock. +type LockProbe struct { + State LockState + // PID is the lock holder's process ID where the platform reports it + // (fcntl F_GETLK on macOS and Linux), else 0. + PID int + Err error +} + +// ProbeLock reports whether another process holds the Zotero profile lock in +// profileDir. It only queries: it never creates, truncates, or locks the file. +func ProbeLock(profileDir string) LockProbe { + return probeLock(profileDir) +} + +// State summarises the two signals. +type State string + +const ( + // StateReady means the connector answered: imports can proceed. + StateReady State = "ready" + // StateStarting means the process holds its profile lock but the + // connector did not answer (see the package doc's transition window). + StateStarting State = "starting" + // StateStopped means neither signal holds. + StateStopped State = "stopped" +) + +// Evidence names the strongest signal behind Status.Running. +type Evidence string + +const ( + EvidenceConnector Evidence = "connector" + EvidenceProfileLock Evidence = "profile_lock" + EvidenceNone Evidence = "none" +) + +// ProfileStatus is one profile directory and its lock. +type ProfileStatus struct { + Path string `json:"path"` + Lock LockState `json:"lock"` + LockPID int `json:"lock_pid,omitempty"` + LockError string `json:"lock_error,omitempty"` +} + +// Status is one presence check. Its JSON shape is the machine contract of +// `zotio desktop status`. +type Status struct { + Running bool `json:"running"` + ConnectorReachable bool `json:"connector_reachable"` + State State `json:"state"` + Evidence Evidence `json:"evidence"` + ConnectorURL string `json:"connector_url,omitempty"` + ConnectorError string `json:"connector_error,omitempty"` + Profiles []ProfileStatus `json:"profiles"` + DiscoveryError string `json:"discovery_error,omitempty"` + DataDir string `json:"data_dir,omitempty"` + CheckedAt time.Time `json:"checked_at"` +} + +// LockHeld reports whether any profile lock was held. +func (s Status) LockHeld() bool { + return slices.ContainsFunc(s.Profiles, func(p ProfileStatus) bool { return p.Lock == LockHeld }) +} + +// Install is the Zotero desktop installation discovery found. +type Install struct { + // Profiles are the profile directories to probe, preferred first. + Profiles []string + // DataDir is the preferred profile's data directory, "" if unresolved. + DataDir string + // Err explains an incomplete discovery: profiles could not be listed, or + // a data directory could not be resolved. + Err error +} + +// WatchDirs lists the directories whose changes can mean Zotero started: +// every profile directory (the lock file) and the data directory (the +// database's WAL files). +func (in Install) WatchDirs() []string { + dirs := slices.Clone(in.Profiles) + if in.DataDir != "" && !slices.Contains(dirs, in.DataDir) { + dirs = append(dirs, in.DataDir) + } + return dirs +} + +// Discover locates Zotero's profile directories and data directory through +// zoteroprefs, the same discovery the stored-upload guard uses. +func Discover() Install { + all, preferred, err := zoteroprefs.Profiles() + if err != nil { + return Install{Err: err} + } + in := Install{Profiles: make([]string, 0, len(all))} + if preferred != "" { + in.Profiles = append(in.Profiles, preferred) + } + for _, dir := range all { + if dir != preferred { + in.Profiles = append(in.Profiles, dir) + } + } + if preferred != "" { + dataDir, err := zoteroprefs.DataDir(preferred) + if err != nil { + in.Err = fmt.Errorf("resolving Zotero data directory: %w", err) + } else { + in.DataDir = dataDir + } + } + return in +} + +// ErrConnectorUnavailable is the Prober.Ping error when no connector can be +// addressed at all (for example a non-local base URL). +var ErrConnectorUnavailable = errors.New("desktop connector unavailable") + +// DefaultPingTimeout bounds one connector ping. The connector runs on +// Zotero's main thread, so a busy Zotero can be slow to answer; a closed one +// refuses the connection at once. +const DefaultPingTimeout = 3 * time.Second + +// Prober checks both presence signals. +type Prober struct { + Install Install + // Ping asks the connector whether it accepts requests. Nil means no + // connector can be addressed; ConnectorErr then says why. + Ping func(ctx context.Context) error + ConnectorURL string + ConnectorErr error + // PingTimeout bounds each ping; zero means DefaultPingTimeout. + PingTimeout time.Duration +} + +// Probe runs one presence check. It never fails: an undecidable signal is +// reported in the Status rather than returned as an error. +func (p *Prober) Probe(ctx context.Context) Status { + st := Status{ + ConnectorURL: p.ConnectorURL, + Profiles: make([]ProfileStatus, 0, len(p.Install.Profiles)), + DataDir: p.Install.DataDir, + } + if p.Install.Err != nil { + st.DiscoveryError = p.Install.Err.Error() + } + for _, dir := range p.Install.Profiles { + lp := ProbeLock(dir) + ps := ProfileStatus{Path: dir, Lock: lp.State, LockPID: lp.PID} + if lp.Err != nil { + ps.LockError = lp.Err.Error() + } + st.Profiles = append(st.Profiles, ps) + } + + if p.Ping == nil { + err := p.ConnectorErr + if err == nil { + err = ErrConnectorUnavailable + } + st.ConnectorError = err.Error() + } else { + timeout := p.PingTimeout + if timeout <= 0 { + timeout = DefaultPingTimeout + } + pingCtx, cancel := context.WithTimeout(ctx, timeout) + err := p.Ping(pingCtx) + cancel() + if err != nil { + st.ConnectorError = err.Error() + } else { + st.ConnectorReachable = true + } + } + + held := st.LockHeld() + st.Running = held || st.ConnectorReachable + switch { + case st.ConnectorReachable: + st.State, st.Evidence = StateReady, EvidenceConnector + case held: + st.State, st.Evidence = StateStarting, EvidenceProfileLock + default: + st.State, st.Evidence = StateStopped, EvidenceNone + } + st.CheckedAt = time.Now().UTC() + return st +} diff --git a/internal/desktop/presence_test.go b/internal/desktop/presence_test.go new file mode 100644 index 00000000..3c32854b --- /dev/null +++ b/internal/desktop/presence_test.go @@ -0,0 +1,178 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +package desktop + +import ( + "context" + "errors" + "os" + "path/filepath" + "runtime" + "testing" + "time" + + "github.com/fsnotify/fsnotify" + + "zotio/internal/zoteroprefs" +) + +func TestProbeLockAbsentFileIsNotCreated(t *testing.T) { + dir := t.TempDir() + if got := ProbeLock(dir); got.State != LockAbsent || got.Err != nil { + t.Fatalf("ProbeLock(empty dir) = %+v, want absent", got) + } + if _, err := os.Stat(filepath.Join(dir, lockFileName)); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("the probe created %s (stat err %v); it must never create the file Zotero owns", lockFileName, err) + } +} + +// The production sequence: Zotero takes the lock and holds it while it runs, +// then releases it on quit. The probe must see all three states, and on +// macOS and Linux name the holder. +func TestProbeLockFollowsAHolderProcessThroughStartAndQuit(t *testing.T) { + dir := t.TempDir() + h := startLockHolder(t, dir) + + got := ProbeLock(dir) + if got.State != LockHeld { + t.Fatalf("ProbeLock while a child holds the lock = %+v, want held", got) + } + if runtime.GOOS != "windows" && got.PID != h.pid { + t.Fatalf("lock holder PID = %d, want the child's %d", got.PID, h.pid) + } + + h.release(t) + got = ProbeLock(dir) + want := LockFree + if runtime.GOOS == "windows" { + want = LockAbsent // delete-on-close removes parent.lock with the last handle + } + if got.State != want || got.Err != nil { + t.Fatalf("ProbeLock after the holder quit = %+v, want %s", got, want) + } +} + +// A probe that raised filesystem events would wake its own watcher, and the +// waiter would re-probe forever: polling by feedback loop. +func TestProbeLockRaisesNoFilesystemEvents(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, lockFileName), nil, 0o600); err != nil { + t.Fatal(err) + } + w, err := fsnotify.NewWatcher() + if err != nil { + t.Fatal(err) + } + defer w.Close() + if err := w.Add(dir); err != nil { + t.Fatal(err) + } + for range 5 { + if got := ProbeLock(dir); got.State != LockFree { + t.Fatalf("ProbeLock = %+v, want free", got) + } + } + select { + case ev := <-w.Events: + t.Fatalf("probing the lock raised %v", ev) + case err := <-w.Errors: + t.Fatalf("watcher error: %v", err) + case <-time.After(300 * time.Millisecond): + } +} + +func TestProbeStatesFollowTheTwoSignals(t *testing.T) { + refused := func(context.Context) error { return errors.New("connection refused") } + answers := func(context.Context) error { return nil } + + t.Run("stopped", func(t *testing.T) { + p := &Prober{Install: Install{Profiles: []string{t.TempDir()}}, Ping: refused} + st := p.Probe(t.Context()) + if st.Running || st.ConnectorReachable || st.State != StateStopped || st.Evidence != EvidenceNone { + t.Fatalf("status = %+v, want stopped with no evidence", st) + } + if st.ConnectorError != "connection refused" { + t.Fatalf("connector_error = %q, want the ping error", st.ConnectorError) + } + }) + + // Process up, connector not yet listening: the transition window. + t.Run("starting", func(t *testing.T) { + dir := t.TempDir() + startLockHolder(t, dir) + p := &Prober{Install: Install{Profiles: []string{dir}}, Ping: refused} + st := p.Probe(t.Context()) + if !st.Running || st.ConnectorReachable || st.State != StateStarting || st.Evidence != EvidenceProfileLock { + t.Fatalf("status = %+v, want running, starting, profile_lock evidence", st) + } + if len(st.Profiles) != 1 || st.Profiles[0].Lock != LockHeld { + t.Fatalf("profiles = %+v, want the one profile held", st.Profiles) + } + }) + + // The connector is the stronger signal: it answers even when discovery + // found no profile (a pinned or unusual install). + t.Run("ready without a profile", func(t *testing.T) { + p := &Prober{Ping: answers, ConnectorURL: "http://127.0.0.1:23119/connector"} + st := p.Probe(t.Context()) + if !st.Running || !st.ConnectorReachable || st.State != StateReady || st.Evidence != EvidenceConnector { + t.Fatalf("status = %+v, want ready on connector evidence", st) + } + if st.ConnectorError != "" || st.Profiles == nil { + t.Fatalf("status = %+v, want no connector_error and an empty (not null) profiles list", st) + } + }) + + t.Run("no addressable connector", func(t *testing.T) { + cause := errors.New("the desktop connector is only available with a local Zotero base URL") + p := &Prober{ConnectorErr: cause} + st := p.Probe(t.Context()) + if st.ConnectorReachable || st.ConnectorError != cause.Error() { + t.Fatalf("status = %+v, want the connector cause reported", st) + } + }) + + // A connector that accepts the connection and never answers must not + // hang the probe past its bound. + t.Run("ping bounded", func(t *testing.T) { + hang := func(ctx context.Context) error { <-ctx.Done(); return ctx.Err() } + p := &Prober{Ping: hang, PingTimeout: 50 * time.Millisecond} + start := time.Now() + st := p.Probe(t.Context()) + if st.ConnectorReachable || time.Since(start) > 2*time.Second { + t.Fatalf("status = %+v after %v, want an unreachable connector within the ping bound", st, time.Since(start)) + } + }) +} + +func TestDiscoverUsesThePinnedProfileAndItsDataDirectory(t *testing.T) { + profile := t.TempDir() + dataDir := filepath.Join(t.TempDir(), "ZoteroData") + prefs := `user_pref("extensions.zotero.useDataDir", true);` + "\n" + + `user_pref("extensions.zotero.dataDir", "` + filepath.ToSlash(dataDir) + `");` + "\n" + if err := os.WriteFile(filepath.Join(profile, "prefs.js"), []byte(prefs), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv(zoteroprefs.ProfileDirEnv, profile) + + in := Discover() + if in.Err != nil { + t.Fatalf("Discover: %v", in.Err) + } + if len(in.Profiles) != 1 || in.Profiles[0] != profile { + t.Fatalf("profiles = %v, want [%s]", in.Profiles, profile) + } + if in.DataDir != dataDir { + t.Fatalf("data dir = %q, want %q", in.DataDir, dataDir) + } + if got := in.WatchDirs(); len(got) != 2 || got[0] != profile || got[1] != in.DataDir { + t.Fatalf("watch dirs = %v, want the profile then the data dir", got) + } +} + +func TestDiscoverReportsABadPinInsteadOfGuessing(t *testing.T) { + t.Setenv(zoteroprefs.ProfileDirEnv, filepath.Join(t.TempDir(), "missing")) + if in := Discover(); in.Err == nil || len(in.Profiles) != 0 { + t.Fatalf("Discover with a pin at a missing directory = %+v, want an error and no profiles", in) + } +} diff --git a/internal/desktop/wait.go b/internal/desktop/wait.go new file mode 100644 index 00000000..a71614bb --- /dev/null +++ b/internal/desktop/wait.go @@ -0,0 +1,227 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +package desktop + +import ( + "context" + "errors" + "fmt" + "time" + + "github.com/fsnotify/fsnotify" +) + +// ErrNoProfile means discovery found no Zotero profile directory, so there +// is nothing whose changes could announce a start, and the connector does +// not answer either. +var ErrNoProfile = errors.New("no Zotero desktop profile directory found") + +// Wait tuning. A package-level default so a caller passing a zero WaitOptions +// gets the documented behaviour. +const ( + // DefaultSettle coalesces a burst of filesystem events into one probe: + // a Zotero start touches the lock file and creates two WAL files within + // about a second. + DefaultSettle = 200 * time.Millisecond + // DefaultConfirmWindow bounds the connector re-checks after the profile + // lock is first seen held. Zotero's connector listens a few seconds after + // the lock; a database upgrade after a Zotero update can take longer, and + // such an upgrade keeps writing the WAL, which raises further events + // after the window closes. + DefaultConfirmWindow = 2 * time.Minute + // DefaultConfirmFirst and DefaultConfirmMax shape the confirm backoff: + // the first re-check comes quickly, later ones at most this far apart. + DefaultConfirmFirst = 250 * time.Millisecond + DefaultConfirmMax = 2 * time.Second +) + +// WaitOptions configures Wait. +type WaitOptions struct { + Prober *Prober + // WatchDirs are watched for changes; see Install.WatchDirs. + WatchDirs []string + + Settle time.Duration + ConfirmWindow time.Duration + ConfirmFirst time.Duration + ConfirmMax time.Duration + + // OnWatching, when set, runs once the watches are installed and the + // first probe found the connector down. Tests use it to know that a + // later filesystem change will be seen. + OnWatching func() +} + +func (o WaitOptions) withDefaults() WaitOptions { + if o.Settle <= 0 { + o.Settle = DefaultSettle + } + if o.ConfirmWindow <= 0 { + o.ConfirmWindow = DefaultConfirmWindow + } + if o.ConfirmFirst <= 0 { + o.ConfirmFirst = DefaultConfirmFirst + } + if o.ConfirmMax < o.ConfirmFirst { + o.ConfirmMax = max(DefaultConfirmMax, o.ConfirmFirst) + } + return o +} + +// Wait blocks until the Zotero connector accepts requests and returns the +// Status that proved it. If the connector already answers, it returns at +// once. +// +// While Zotero is closed nothing runs on a timer: Wait sleeps on filesystem +// notifications for the profile and data directories and probes only after +// a change settles. Once the profile lock is seen held, the connector is +// re-checked on a capped backoff for ConfirmWindow, because the connector +// listens seconds after the lock and its start writes no file. If the window +// passes with the lock held and the connector still silent (connector +// disabled, another port, a hung Zotero), Wait goes back to sleeping on +// events; a new confirm window opens only when the lock is released and +// taken again. +// +// On cancellation Wait returns the last Status and context.Cause(ctx), so a +// caller that set a deadline with context.WithTimeoutCause can tell a +// timeout from an interrupt. +func Wait(ctx context.Context, opts WaitOptions) (Status, error) { + if opts.Prober == nil { + return Status{}, errors.New("desktop.Wait: nil Prober") + } + o := opts.withDefaults() + + // Subscribe before the first probe: a Zotero that starts between the + // probe and the subscription would otherwise raise its events unseen. + watcher, watchErr := newWatcher(o.WatchDirs) + if watcher != nil { + defer watcher.Close() + } + + st := o.Prober.Probe(ctx) + if st.ConnectorReachable { + return st, nil + } + if watcher == nil { + if watchErr == nil { + return st, ErrNoProfile + } + return st, watchErr + } + if o.OnWatching != nil { + o.OnWatching() + } + + var ( + settle *time.Timer + confirm *time.Timer + confirmDeadline time.Time + backoff time.Duration + lockSeen = st.LockHeld() + ) + stopTimer := func(t **time.Timer) { + if *t != nil { + (*t).Stop() + *t = nil + } + } + defer stopTimer(&settle) + defer stopTimer(&confirm) + startConfirm := func() { + stopTimer(&confirm) + confirmDeadline = time.Now().Add(o.ConfirmWindow) + backoff = o.ConfirmFirst + confirm = time.NewTimer(backoff) + } + if lockSeen { + startConfirm() + } + timerC := func(t *time.Timer) <-chan time.Time { + if t == nil { + return nil + } + return t.C + } + armSettle := func() { + if settle == nil { + settle = time.NewTimer(o.Settle) + } + } + + for { + select { + case <-ctx.Done(): + return st, context.Cause(ctx) + + case _, ok := <-watcher.Events: + if !ok { + return st, errors.New("filesystem watcher closed") + } + armSettle() + + case _, ok := <-watcher.Errors: + if !ok { + return st, errors.New("filesystem watcher closed") + } + // An overflow or read error means events may have been lost: + // probe as though one arrived rather than trusting the silence. + armSettle() + + case <-timerC(settle): + settle = nil + st = o.Prober.Probe(ctx) + if st.ConnectorReachable { + return st, nil + } + held := st.LockHeld() + switch { + case held && !lockSeen: + startConfirm() + case !held: + stopTimer(&confirm) + } + lockSeen = held + + case <-timerC(confirm): + confirm = nil + st = o.Prober.Probe(ctx) + if st.ConnectorReachable { + return st, nil + } + lockSeen = st.LockHeld() + remaining := time.Until(confirmDeadline) + if !lockSeen || remaining <= 0 { + continue + } + backoff = min(backoff*2, o.ConfirmMax) + confirm = time.NewTimer(min(backoff, remaining)) + } + } +} + +// newWatcher watches every directory it can. It returns a nil watcher when +// none could be watched, with the reason, or with a nil error when there +// was nothing to watch. +func newWatcher(dirs []string) (*fsnotify.Watcher, error) { + if len(dirs) == 0 { + return nil, nil + } + w, err := fsnotify.NewWatcher() + if err != nil { + return nil, fmt.Errorf("starting the filesystem watcher: %w", err) + } + var errs []error + added := 0 + for _, dir := range dirs { + if err := w.Add(dir); err != nil { + errs = append(errs, fmt.Errorf("watching %s: %w", dir, err)) + continue + } + added++ + } + if added == 0 { + _ = w.Close() + return nil, errors.Join(errs...) + } + return w, nil +} diff --git a/internal/desktop/wait_test.go b/internal/desktop/wait_test.go new file mode 100644 index 00000000..1305bc33 --- /dev/null +++ b/internal/desktop/wait_test.go @@ -0,0 +1,273 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +package desktop + +import ( + "context" + "errors" + "os" + "path/filepath" + "sync" + "testing" + "time" +) + +// fakeConnector stands in for Zotero's /connector/ping and counts every +// probe, which is how these tests tell event-driven waiting from polling. +type fakeConnector struct { + mu sync.Mutex + up bool + calls int +} + +func (f *fakeConnector) ping(context.Context) error { + f.mu.Lock() + defer f.mu.Unlock() + f.calls++ + if f.up { + return nil + } + return errors.New("dial tcp 127.0.0.1:23119: connect: connection refused") +} + +func (f *fakeConnector) setUp() { + f.mu.Lock() + f.up = true + f.mu.Unlock() +} + +func (f *fakeConnector) count() int { + f.mu.Lock() + defer f.mu.Unlock() + return f.calls +} + +// install is a closed Zotero: a profile directory and a data directory. +type install struct { + profile, data string +} + +func newInstall(t *testing.T) install { + t.Helper() + root := t.TempDir() + in := install{profile: filepath.Join(root, "Profiles", "abcd1234.default"), data: filepath.Join(root, "Zotero")} + for _, dir := range []string{in.profile, in.data} { + if err := os.MkdirAll(dir, 0o750); err != nil { + t.Fatal(err) + } + } + return in +} + +type waitResult struct { + st Status + err error +} + +// startWait runs Wait in the background and returns once its watches are +// installed, so a later filesystem change is guaranteed to be seen. +func startWait(t *testing.T, ctx context.Context, in install, conn *fakeConnector, tune func(*WaitOptions)) <-chan waitResult { + t.Helper() + watching := make(chan struct{}) + opts := WaitOptions{ + Prober: &Prober{Install: Install{Profiles: []string{in.profile}, DataDir: in.data}, Ping: conn.ping}, + WatchDirs: []string{in.profile, in.data}, + Settle: 20 * time.Millisecond, + OnWatching: func() { close(watching) }, + } + if tune != nil { + tune(&opts) + } + done := make(chan waitResult, 1) + go func() { + st, err := Wait(ctx, opts) + done <- waitResult{st, err} + }() + select { + case <-watching: + case r := <-done: + t.Fatalf("Wait returned before watching: %+v, %v", r.st, r.err) + case <-time.After(10 * time.Second): + t.Fatal("Wait never installed its watches") + } + return done +} + +func awaitResult(t *testing.T, done <-chan waitResult) waitResult { + t.Helper() + select { + case r := <-done: + return r + case <-time.After(10 * time.Second): + t.Fatal("Wait did not return") + return waitResult{} + } +} + +func assertStillWaiting(t *testing.T, done <-chan waitResult, d time.Duration) { + t.Helper() + select { + case r := <-done: + t.Fatalf("Wait returned early: %+v, %v", r.st, r.err) + case <-time.After(d): + } +} + +func TestWaitReturnsAtOnceWhenTheConnectorAlreadyAnswers(t *testing.T) { + conn := &fakeConnector{up: true} + // No profile and nothing to watch: an answering connector needs neither. + st, err := Wait(t.Context(), WaitOptions{Prober: &Prober{Ping: conn.ping}}) + if err != nil || !st.ConnectorReachable || st.State != StateReady { + t.Fatalf("Wait = %+v, %v; want ready at once", st, err) + } + if conn.count() != 1 { + t.Fatalf("pings = %d, want exactly 1", conn.count()) + } +} + +func TestWaitWithNoProfileAndNoConnectorFailsWithErrNoProfile(t *testing.T) { + conn := &fakeConnector{} + st, err := Wait(t.Context(), WaitOptions{Prober: &Prober{Ping: conn.ping}}) + if !errors.Is(err, ErrNoProfile) || st.Running { + t.Fatalf("Wait = %+v, %v; want ErrNoProfile", st, err) + } +} + +// While Zotero is closed nothing may run on a timer: after the first probe +// the connector is not asked again until the filesystem changes. +func TestWaitSleepsWhileClosedAndWakesOnAFilesystemEvent(t *testing.T) { + in := newInstall(t) + conn := &fakeConnector{} + done := startWait(t, t.Context(), in, conn, nil) + + assertStillWaiting(t, done, 400*time.Millisecond) + if got := conn.count(); got != 1 { + t.Fatalf("pings while Zotero is closed = %d, want 1 (no polling)", got) + } + + conn.setUp() + // Zotero opening its database creates the WAL file in the data dir. + if err := os.WriteFile(filepath.Join(in.data, "zotero.sqlite-wal"), []byte("wal"), 0o600); err != nil { + t.Fatal(err) + } + r := awaitResult(t, done) + if r.err != nil || r.st.State != StateReady { + t.Fatalf("Wait = %+v, %v; want ready after the event", r.st, r.err) + } +} + +// The measured Zotero start: the profile lock is taken first, the connector +// listens seconds later, and starting the connector writes no file. Wait +// must bridge that gap on its own without a further event. +func TestWaitConfirmsTheConnectorAfterTheLockWithoutAFurtherEvent(t *testing.T) { + in := newInstall(t) + conn := &fakeConnector{} + done := startWait(t, t.Context(), in, conn, func(o *WaitOptions) { + o.ConfirmFirst, o.ConfirmMax, o.ConfirmWindow = 10*time.Millisecond, 40*time.Millisecond, 10*time.Second + }) + + h := startLockHolder(t, in.profile) // Zotero takes its profile lock + deadline := time.Now().Add(10 * time.Second) + for conn.count() < 4 { // the lock event's probe plus confirm re-checks + if time.Now().After(deadline) { + t.Fatalf("pings after the lock = %d, want confirm re-checks", conn.count()) + } + time.Sleep(5 * time.Millisecond) + } + conn.setUp() // the connector starts listening; no file changes + + r := awaitResult(t, done) + if r.err != nil || r.st.State != StateReady || !r.st.LockHeld() { + t.Fatalf("Wait = %+v, %v; want ready with the lock held", r.st, r.err) + } + h.release(t) +} + +// Zotero running with its connector disabled keeps the lock held and never +// answers. The confirm re-checks must stop when the window closes; after +// that only a filesystem event may cause a probe. +func TestWaitStopsReCheckingWhenTheConfirmWindowCloses(t *testing.T) { + in := newInstall(t) + startLockHolder(t, in.profile) + conn := &fakeConnector{} + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := startWait(t, ctx, in, conn, func(o *WaitOptions) { + o.ConfirmFirst, o.ConfirmMax, o.ConfirmWindow = 10*time.Millisecond, 20*time.Millisecond, 150*time.Millisecond + }) + + time.Sleep(400 * time.Millisecond) // well past the window + settled := conn.count() + if settled < 3 { + t.Fatalf("pings during the confirm window = %d, want re-checks", settled) + } + assertStillWaiting(t, done, 400*time.Millisecond) + if got := conn.count(); got != settled { + t.Fatalf("pings after the confirm window = %d, then %d; want no polling", settled, got) + } + + if err := os.WriteFile(filepath.Join(in.data, "zotero.sqlite-wal"), []byte("wal"), 0o600); err != nil { + t.Fatal(err) + } + deadline := time.Now().Add(5 * time.Second) + for conn.count() == settled { + if time.Now().After(deadline) { + t.Fatal("a filesystem event after the window caused no probe") + } + time.Sleep(5 * time.Millisecond) + } + cancel() + if r := awaitResult(t, done); !errors.Is(r.err, context.Canceled) { + t.Fatalf("Wait after cancel = %+v, %v; want context.Canceled", r.st, r.err) + } +} + +func TestWaitTimeoutReturnsItsCause(t *testing.T) { + in := newInstall(t) + conn := &fakeConnector{} + errTimeout := errors.New("wait timed out") + ctx, cancel := context.WithTimeoutCause(t.Context(), 150*time.Millisecond, errTimeout) + defer cancel() + done := startWait(t, ctx, in, conn, nil) + + r := awaitResult(t, done) + if !errors.Is(r.err, errTimeout) { + t.Fatalf("Wait = %+v, %v; want the timeout cause", r.st, r.err) + } + if r.st.State != StateStopped || r.st.CheckedAt.IsZero() { + t.Fatalf("status at timeout = %+v, want the last (stopped) probe", r.st) + } +} + +func TestWaitCancellationReturnsPromptly(t *testing.T) { + in := newInstall(t) + conn := &fakeConnector{} + ctx, cancel := context.WithCancel(t.Context()) + done := startWait(t, ctx, in, conn, nil) + + start := time.Now() + cancel() + r := awaitResult(t, done) + if !errors.Is(r.err, context.Canceled) { + t.Fatalf("Wait = %+v, %v; want context.Canceled", r.st, r.err) + } + if elapsed := time.Since(start); elapsed > time.Second { + t.Fatalf("Wait took %v to honour cancellation", elapsed) + } +} + +// A data directory that does not exist (Zotero not yet run, or moved) must +// not stop the wait while the profile directory can still be watched. +func TestWaitWatchesWhatExists(t *testing.T) { + in := newInstall(t) + if err := os.Remove(in.data); err != nil { + t.Fatal(err) + } + conn := &fakeConnector{} + done := startWait(t, t.Context(), in, conn, nil) + conn.setUp() + startLockHolder(t, in.profile) + if r := awaitResult(t, done); r.err != nil || r.st.State != StateReady { + t.Fatalf("Wait = %+v, %v; want ready from a profile-directory event", r.st, r.err) + } +} diff --git a/internal/mcp/testdata/surface_mirror.golden.json b/internal/mcp/testdata/surface_mirror.golden.json index 3d082e63..f1ba4592 100644 --- a/internal/mcp/testdata/surface_mirror.golden.json +++ b/internal/mcp/testdata/surface_mirror.golden.json @@ -707,6 +707,15 @@ "type": "object" } }, + { + "name": "desktop_status", + "description": "Report whether Zotero desktop is running and whether its connector accepts\nrequests. Cheap and local: it reads the profile lock of every discovered\nZotero profile and sends one ping to the local connector. It exits 0 whatever\nit finds; read the fields, not the exit code.\n\nTwo signals, reported separately:\n\n running Zotero's process is up: another process holds the\n profile lock (.parentlock via fcntl on macOS and Linux,\n parent.lock opened exclusively on Windows), or the\n connector answered.\n connector_reachable GET \u003cconnector\u003e/ping answered 200 during this check.\n Imports and every other connector write need this.\n\nstate is \"ready\" when the connector answers, \"stopped\" when neither signal\nholds, and \"starting\" when the process holds its lock but the connector does\nnot answer. Zotero takes the lock about 3s after launch and its connector\nlistens a few seconds later, so \"starting\" is normal briefly after a launch;\nit persists if the connector is disabled (Settings -\u003e Advanced -\u003e \"Allow other\napplications to communicate with Zotero\"), moved to another port, or Zotero\nis hung. evidence names the strongest signal: connector, profile_lock, none.\n\nProfiles are discovered from profiles.ini in the platform's Zotero directory\n(ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js.", + "inputSchema": { + "properties": {}, + "required": [], + "type": "object" + } + }, { "name": "export", "description": "Export paginated API data to a local file. Output defaults to JSONL\n(one JSON object per line, streaming-friendly); JSON output is available for\nbackwards-compatible resource exports.", diff --git a/internal/zoteroprefs/presence_discovery_test.go b/internal/zoteroprefs/presence_discovery_test.go new file mode 100644 index 00000000..4fa5b397 --- /dev/null +++ b/internal/zoteroprefs/presence_discovery_test.go @@ -0,0 +1,100 @@ +// Copyright 2026 OrgMentem. Licensed under MIT. See LICENSE. + +package zoteroprefs + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestDataDirFollowsUseDataDirLikeZotero(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) + custom := filepath.Join(t.TempDir(), "Research", "Zotero") + quoted := strings.ReplaceAll(custom, `\`, `\\`) + + cases := []struct { + name string + prefs string + want string + }{ + {"chosen directory", `user_pref("extensions.zotero.useDataDir", true);` + "\n" + + `user_pref("extensions.zotero.dataDir", "` + quoted + `");`, custom}, + // Zotero.DataDirectory.init ignores dataDir unless useDataDir is set. + {"dataDir without useDataDir", `user_pref("extensions.zotero.dataDir", "` + quoted + `");`, filepath.Join(home, "Zotero")}, + {"useDataDir false", `user_pref("extensions.zotero.useDataDir", false);` + "\n" + + `user_pref("extensions.zotero.dataDir", "` + quoted + `");`, filepath.Join(home, "Zotero")}, + {"no choice recorded", `user_pref("extensions.zotero.sync.storage.protocol", "zotero");`, filepath.Join(home, "Zotero")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := DataDir(writeProfile(t, tc.prefs)) + if err != nil { + t.Fatalf("DataDir: %v", err) + } + if got != tc.want { + t.Fatalf("DataDir = %q, want %q", got, tc.want) + } + }) + } + + t.Run("no prefs.js", func(t *testing.T) { + got, err := DataDir(t.TempDir()) + if err != nil || got != filepath.Join(home, "Zotero") { + t.Fatalf("DataDir(no prefs.js) = %q, %v; want the default", got, err) + } + }) +} + +// A pre-5.0 persistent descriptor (or any relative value) names no location +// this package can use; guessing one would watch the wrong directory. +func TestDataDirRefusesANonAbsoluteChoice(t *testing.T) { + dir := writeProfile(t, `user_pref("extensions.zotero.useDataDir", true);`+"\n"+ + `user_pref("extensions.zotero.dataDir", "Zotero");`) + if got, err := DataDir(dir); err == nil { + t.Fatalf("DataDir = %q, nil; want an error for a relative dataDir", got) + } +} + +// Snap and Flatpak give the Zotero process its own home, so the default data +// directory sits beside the sandboxed .zotero tree, not in the user's home. +func TestDefaultDataDirUsesTheSandboxHomeOnLinux(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + setGOOS(t, "linux") + snapHome := filepath.Join(home, "snap", "zotero", "common") + profile := filepath.Join(snapHome, ".zotero", "zotero", "aaaaaaaa.default") + if err := os.MkdirAll(profile, 0o750); err != nil { + t.Fatal(err) + } + got, err := DataDir(profile) + if err != nil { + t.Fatalf("DataDir: %v", err) + } + if want := filepath.Join(snapHome, "Zotero"); got != want { + t.Fatalf("DataDir(snap profile) = %q, want %q", got, want) + } +} + +func TestProfilesHonoursThePinAndRefusesABadOne(t *testing.T) { + dir := t.TempDir() + t.Setenv(ProfileDirEnv, dir) + all, preferred, err := Profiles() + if err != nil || len(all) != 1 || all[0] != dir || preferred != dir { + t.Fatalf("Profiles() with a pin = %v, %q, %v; want exactly the pin", all, preferred, err) + } + + // Unlike Load, a pin need not hold a prefs.js: the lock file lives in the + // directory whatever the preferences say. It must exist, though. + t.Setenv(ProfileDirEnv, filepath.Join(dir, "missing")) + if all, _, err := Profiles(); err == nil { + t.Fatalf("Profiles() with a pin at a missing directory = %v, nil; want an error", all) + } + t.Setenv(ProfileDirEnv, "relative") + if _, _, err := Profiles(); err == nil || !strings.Contains(err.Error(), "absolute") { + t.Fatalf("Profiles() with a relative pin error = %v, want it to say the pin must be absolute", err) + } +} diff --git a/internal/zoteroprefs/zoteroprefs.go b/internal/zoteroprefs/zoteroprefs.go index 3be5e4f4..d8e7d293 100644 --- a/internal/zoteroprefs/zoteroprefs.go +++ b/internal/zoteroprefs/zoteroprefs.go @@ -11,6 +11,11 @@ // the desktop's preference is the only way to notice that mismatch before // consuming a plan the operator does not use. // +// The same profile discovery also serves Zotero desktop presence detection +// (internal/desktop): Profiles lists the profile directories whose lock file +// says whether Zotero is running, and DataDir names the data directory whose +// database files change when it starts. +// // Only reads. Nothing here writes to a Zotero profile. // // # Two axes, not one @@ -70,6 +75,10 @@ const ( prefStorageGroupsEnabled = "extensions.zotero.sync.storage.groups.enabled" prefStorageURL = "extensions.zotero.sync.storage.url" prefStorageVerified = "extensions.zotero.sync.storage.verified" + // Zotero.DataDirectory.init reads dataDir only when useDataDir is true; + // otherwise it uses its default location. + prefUseDataDir = "extensions.zotero.useDataDir" + prefDataDir = "extensions.zotero.dataDir" ) // prefs.js is a small generated file. This bound keeps a corrupt or hostile @@ -333,20 +342,11 @@ func (f FileStorage) WebDAVHost() string { // to WebDAV and sets Ambiguous. That keeps the dangerous direction — assuming // Zotero's cloud when the running profile actually uses WebDAV — closed. func Load() (FileStorage, error) { - if override := strings.TrimSpace(os.Getenv(ProfileDirEnv)); override != "" { - // A relative pin resolves against whatever directory zotio starts in, - // so one setting names different profiles from a shell and from an MCP - // host, and can name a prefs.js someone else placed there. Refuse it - // rather than guess: this pin decides where stored uploads may go. - if !filepath.IsAbs(override) { - hint := "" - if override == "~" || strings.HasPrefix(override, "~/") { - // MCP host configs and service files pass env values verbatim; - // only an interactive shell expands "~". - hint = "; \"~\" is not expanded here, write the full home path" - } - return FileStorage{}, fmt.Errorf("%s must be an absolute path, got %q%s", ProfileDirEnv, override, hint) - } + override, pinned, err := pinnedProfileDir() + if err != nil { + return FileStorage{}, err + } + if pinned { fs, err := LoadProfile(override) if err != nil { return FileStorage{}, err @@ -369,6 +369,105 @@ func Load() (FileStorage, error) { return loadAcross(dirs, preferred) } +// pinnedProfileDir returns the ProfileDirEnv pin, if one is set. +// +// A relative pin resolves against whatever directory zotio starts in, so one +// setting names different profiles from a shell and from an MCP host, and can +// name a prefs.js someone else placed there. It is refused rather than +// guessed: the pin decides where stored uploads may go, and which lock file +// says Zotero is running. +func pinnedProfileDir() (dir string, pinned bool, err error) { + override := strings.TrimSpace(os.Getenv(ProfileDirEnv)) + if override == "" { + return "", false, nil + } + if !filepath.IsAbs(override) { + hint := "" + if override == "~" || strings.HasPrefix(override, "~/") { + // MCP host configs and service files pass env values verbatim; + // only an interactive shell expands "~". + hint = "; \"~\" is not expanded here, write the full home path" + } + return "", false, fmt.Errorf("%s must be an absolute path, got %q%s", ProfileDirEnv, override, hint) + } + return override, true, nil +} + +// Profiles lists the Zotero desktop profile directories this machine +// exposes, plus the one to prefer for reporting. +// +// A ProfileDirEnv pin wins and names exactly one profile; a pin at a +// directory that does not exist is an error, because the operator asserted +// it. Otherwise discovery runs across every platform root, as Load does. A +// machine with no Zotero profile returns no directories and no error: +// callers decide whether absence is fatal. +func Profiles() (all []string, preferred string, err error) { + override, pinned, err := pinnedProfileDir() + if err != nil { + return nil, "", err + } + if pinned { + if !profileDirLooksUsable(override) { + return nil, "", fmt.Errorf("%s points at %s, which is not a directory", ProfileDirEnv, override) + } + return []string{override}, override, nil + } + return discoverProfiles() +} + +// DataDir returns the Zotero data directory (the one holding zotero.sqlite) +// that the profile at profileDir uses. +// +// It mirrors Zotero.DataDirectory.init: extensions.zotero.dataDir applies +// only when extensions.zotero.useDataDir is true; otherwise Zotero uses its +// default, a "Zotero" folder in the home directory of the Zotero process. +// Snap and Flatpak packaging give that process a different home, so on +// Linux the home is taken from the profile path when the profile sits under +// a ".zotero/zotero" root. A profile with no prefs.js has never recorded a +// choice, so it gets the default too. +func DataDir(profileDir string) (string, error) { + values, _, err := readPrefs(profileDir) + if err != nil { + return "", err + } + if use, ok := values[prefUseDataDir].asBool(); ok && use { + if v, ok := values[prefDataDir]; ok && !v.undecodable { + dir := strings.TrimSpace(v.str) + // Zotero before 5.0 stored a platform "persistent descriptor" + // here and converts it to a path on its next start. Anything + // that is not an absolute path is not a location this package + // can name. + if !filepath.IsAbs(dir) { + return "", fmt.Errorf("Zotero pref %s in %s is not an absolute path", prefDataDir, profileDir) + } + return filepath.Clean(dir), nil + } + } + return defaultDataDir(profileDir) +} + +// defaultDataDir is Zotero's default data directory for a profile that has +// not chosen one: /Zotero, where home is the Zotero process's home. +func defaultDataDir(profileDir string) (string, error) { + switch goos { + case "darwin", "windows": + default: + // Native, Snap and Flatpak profiles all sit under /.zotero/zotero + // for the home the Zotero process sees (see profileRoots). + sep := string(filepath.Separator) + marker := sep + filepath.Join(".zotero", "zotero") + sep + clean := filepath.Clean(profileDir) + if i := strings.LastIndex(clean+sep, marker); i > 0 { + return filepath.Join(clean[:i], "Zotero"), nil + } + } + home, err := cliutil.HomeDir() + if err != nil { + return "", fmt.Errorf("resolving home dir: %w", err) + } + return filepath.Join(home, "Zotero"), nil +} + // LoadAcrossForTest exposes multi-profile folding to tests in other // packages, which otherwise cannot construct an ambiguous, multi-profile // reading without hand-setting fields — exactly the shortcut that let a @@ -468,59 +567,9 @@ func riskRank(m StorageMode) int { // LoadProfile reads file-storage configuration from one profile directory. func LoadProfile(profileDir string) (FileStorage, error) { - path := filepath.Join(profileDir, "prefs.js") - // Only a regular file is read. os.Open on a FIFO blocks in open(2) until a - // writer appears, which no read limit can bound, and the guard is consulted - // on every mutating invocation — a hung zotio is worse than an unread - // profile. The residual stat/open race is benign: the worst case is the - // behaviour we would have had anyway. - // #nosec G703 -- same taint source and read-only use as the Open below. - info, err := os.Lstat(path) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return FileStorage{}, nil - } - return FileStorage{}, fmt.Errorf("reading Zotero prefs %s: %w", path, err) - } - if !info.Mode().IsRegular() { - return FileStorage{}, fmt.Errorf("Zotero prefs %s is not a regular file (%s)", path, info.Mode().Type()) - } - // #nosec G304,G703 -- profileDir comes from platform discovery or the - // operator's own ZOTERO_PROFILE_DIR, and this only opens it for reading. - f, err := os.Open(path) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return FileStorage{}, nil - } - return FileStorage{}, fmt.Errorf("reading Zotero prefs %s: %w", path, err) - } - defer f.Close() - - // Read one byte past the cap so truncation is detectable: silently parsing - // a prefix would turn a present preference into an absent one, and absent - // preferences fall back to Zotero's cloud defaults. - data, err := io.ReadAll(io.LimitReader(f, maxPrefsFileBytes+1)) - if err != nil { - return FileStorage{}, fmt.Errorf("reading Zotero prefs %s: %w", path, err) - } - if len(data) > maxPrefsFileBytes { - return FileStorage{}, fmt.Errorf("Zotero prefs %s exceeds %d bytes; refusing to read a partial preference set", path, maxPrefsFileBytes) - } - // Zotero (a Firefox-based application) always writes prefs.js as UTF-8. A - // differently encoded file — most plausibly UTF-16, from a profile a - // stray tool has touched — makes every "user_pref(" prefix match fail, - // since that ASCII byte sequence never starts a UTF-16 line. Every - // preference would then read as absent, which resolves to Zotero's cloud - // defaults: a confident wrong answer built from a decode failure, not - // from evidence. Surfacing the encoding mismatch as an error instead lets - // the caller treat it as the evaluation failure it is. - if reason := notUTF8Reason(data); reason != "" { - return FileStorage{}, fmt.Errorf("Zotero prefs %s is not readable as UTF-8 (%s)", path, reason) - } - - values, err := parsePrefs(bytes.NewReader(data)) - if err != nil { - return FileStorage{}, fmt.Errorf("parsing Zotero prefs %s: %w", path, err) + values, found, err := readPrefs(profileDir) + if err != nil || !found { + return FileStorage{}, err } // Defaults come from Zotero's defaults/preferences/zotero.js: file syncing @@ -594,6 +643,67 @@ func LoadProfile(profileDir string) (FileStorage, error) { return fs, nil } +// readPrefs reads and decodes one profile's prefs.js. A profile directory +// with no prefs.js returns found == false and no error: Zotero has not +// necessarily run there yet. +func readPrefs(profileDir string) (values map[string]prefValue, found bool, err error) { + path := filepath.Join(profileDir, "prefs.js") + // Only a regular file is read. os.Open on a FIFO blocks in open(2) until a + // writer appears, which no read limit can bound, and the guard is consulted + // on every mutating invocation — a hung zotio is worse than an unread + // profile. The residual stat/open race is benign: the worst case is the + // behaviour we would have had anyway. + // #nosec G703 -- same taint source and read-only use as the Open below. + info, err := os.Lstat(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, false, nil + } + return nil, false, fmt.Errorf("reading Zotero prefs %s: %w", path, err) + } + if !info.Mode().IsRegular() { + return nil, false, fmt.Errorf("Zotero prefs %s is not a regular file (%s)", path, info.Mode().Type()) + } + // #nosec G304,G703 -- profileDir comes from platform discovery or the + // operator's own ZOTERO_PROFILE_DIR, and this only opens it for reading. + f, err := os.Open(path) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return nil, false, nil + } + return nil, false, fmt.Errorf("reading Zotero prefs %s: %w", path, err) + } + defer f.Close() + + // Read one byte past the cap so truncation is detectable: silently parsing + // a prefix would turn a present preference into an absent one, and absent + // preferences fall back to Zotero's cloud defaults. + data, err := io.ReadAll(io.LimitReader(f, maxPrefsFileBytes+1)) + if err != nil { + return nil, false, fmt.Errorf("reading Zotero prefs %s: %w", path, err) + } + if len(data) > maxPrefsFileBytes { + return nil, false, fmt.Errorf("Zotero prefs %s exceeds %d bytes; refusing to read a partial preference set", path, maxPrefsFileBytes) + } + // Zotero (a Firefox-based application) always writes prefs.js as UTF-8. A + // differently encoded file — most plausibly UTF-16, from a profile a + // stray tool has touched — makes every "user_pref(" prefix match fail, + // since that ASCII byte sequence never starts a UTF-16 line. Every + // preference would then read as absent, which resolves to Zotero's cloud + // defaults: a confident wrong answer built from a decode failure, not + // from evidence. Surfacing the encoding mismatch as an error instead lets + // the caller treat it as the evaluation failure it is. + if reason := notUTF8Reason(data); reason != "" { + return nil, false, fmt.Errorf("Zotero prefs %s is not readable as UTF-8 (%s)", path, reason) + } + + values, err = parsePrefs(bytes.NewReader(data)) + if err != nil { + return nil, false, fmt.Errorf("parsing Zotero prefs %s: %w", path, err) + } + return values, true, nil +} + // goos selects Zotero's per-platform profile layout. A package variable // rather than a direct runtime.GOOS reference so tests can drive the // Linux/Unix discovery path — including the Snap and Flatpak candidates — From 7f375b65387b9e8ea2dc36c94e23549e6687ab90 Mon Sep 17 00:00:00 2001 From: enieuwy <121954036+enieuwy@users.noreply.github.com> Date: Thu, 24 Sep 2026 15:27:49 +0800 Subject: [PATCH 2/5] feat(desktop): report a hung or connector-off Zotero as its own state A Zotero that holds its profile lock but whose connector cannot take requests was reported as "starting" forever, so a caller told the user to wait, or to open a Zotero that was already open. Seen live: the process held its lock for 1h40m while /connector/ping accepted the connection and never answered, and the window's accessibility tree failed too. "starting" now lasts only for a 2-minute startup window measured from the lock time. Mozilla's lock open truncates .parentlock, so its mtime is the lock time (measured 3s after process start on a file created years earlier); Windows recreates parent.lock at each launch. Past the window the state is "unresponsive" when the connector port accepts and does not answer usefully (timeout, reset, non-200), or "connector_off" when the dial is refused. profiles[].lock_since reports the lock time. desktop wait no longer goes silent after its confirm window: when Zotero is, or becomes, unresponsive or connector_off it returns at once with exit 15 and that state as outcome, because no filesystem change announces a recovery; the caller tells the user and re-waits. Closed Zotero still sleeps on events with no polling. --- CHANGELOG.md | 32 ++-- README.md | 2 +- SKILL.md | 3 +- dev/zotero-api-coverage.md | 6 +- docs/reference/commands.md | 78 ++++++--- internal/cli/desktop.go | 74 +++++++-- internal/cli/desktop_test.go | 45 +++++ internal/cli/helpers.go | 7 + internal/desktop/lock_unix.go | 3 +- internal/desktop/lock_windows.go | 11 +- internal/desktop/presence.go | 157 +++++++++++++++--- internal/desktop/presence_test.go | 106 ++++++++++++ internal/desktop/wait.go | 151 +++++++++-------- internal/desktop/wait_test.go | 89 ++++++---- .../mcp/testdata/surface_mirror.golden.json | 2 +- 15 files changed, 588 insertions(+), 178 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac613891..92c1cc97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,21 +12,31 @@ Notable changes to zotio. Format follows [Keep a Changelog](https://keepachangel Windows) and sends one ping to the local connector. `running` means the process is up (lock held, or the connector answered); `connector_reachable` means the connector accepts requests now, which imports need. `state` is - `ready`, `starting` (process up, connector not answering: normal for a few - seconds after launch, and permanent if the connector is disabled) or - `stopped`. It exits 0 whatever it finds. + `ready`; `starting` (lock held, connector silent, lock younger than the + 2-minute startup window); `unresponsive` (past the window, the connector + port accepts connections but does not answer: Zotero is open but hung); + `connector_off` (past the window, nothing listens on the connector port); + or `stopped`. The lock age is the lock file's modification time, which + Zotero resets when it takes the lock (`profiles[].lock_since`). It exits 0 + whatever it finds. - **`zotio desktop wait` blocks until Zotero's connector accepts requests.** It returns at once if the connector already answers. While Zotero is closed it sleeps on filesystem notifications for the profile and data directories - instead of polling, and probes only after a change; once the lock is held it - re-checks the connector on a capped backoff for up to 2 minutes. `--timeout` - bounds the wait (exit 14, `outcome: "timeout"`); no discoverable profile - exits 9 (`outcome: "no_profile"`); `--watch-stdin` exits when a supervisor's - pipe closes. It is hidden from the MCP surface because it blocks. The - notifications come from the new dependency `github.com/fsnotify/fsnotify` - (BSD-3-Clause; inotify, kqueue, ReadDirectoryChangesW), whose only - dependency, `golang.org/x/sys`, was already linked. + instead of polling, and probes only after a change; while Zotero is starting + it re-checks the connector on a capped backoff. When Zotero is, or becomes, + `unresponsive` or `connector_off`, it returns at once with exit 15 and that + state as `outcome`, because nothing on disk announces a recovery. + `--timeout` bounds the wait (exit 14, `outcome: "timeout"`); no + discoverable profile exits 9 (`outcome: "no_profile"`); `--watch-stdin` + exits when a supervisor's pipe closes. It is hidden from the MCP surface + because it blocks. The notifications come from the new dependency + `github.com/fsnotify/fsnotify` (BSD-3-Clause; inotify, kqueue, + ReadDirectoryChangesW), whose only dependency, `golang.org/x/sys`, was + already linked. - **Exit code 14: a bounded wait timed out.** Nothing failed; wait again. +- **Exit code 15: Zotero desktop is open but its connector cannot take + requests** (`desktop wait`: hung, or connector disabled). Tell the user; + waiting longer does not help until something changes. ## [0.27.0] — 2026-09-23 diff --git a/README.md b/README.md index cff88dae..d83b1d13 100644 --- a/README.md +++ b/README.md @@ -308,7 +308,7 @@ format = "obsidian" # or "logseq" collection:KEY tag:NAME query:TEXT item:KEY saved-search:KEY (needs live desktop) ``` -**Exit codes:** `0` ok · `2` usage · `3` not-found · `4` auth · `5` API · `7` rate-limited · `9` precondition/setup (including *another writer holds the lock* — retry) · `10` config · `11` quality-gate failed · `12` freshness-gate failed · `13` degraded — incomplete: part of a read was unreadable, or part of a batched write was rejected after other elements succeeded. Output is not guaranteed; read the reported failures and reconcile before retrying. · `14` timed out — a bounded wait (`desktop wait --timeout`) ended before Zotero's connector answered; wait again. +**Exit codes:** `0` ok · `2` usage · `3` not-found · `4` auth · `5` API · `7` rate-limited · `9` precondition/setup (including *another writer holds the lock* — retry) · `10` config · `11` quality-gate failed · `12` freshness-gate failed · `13` degraded — incomplete: part of a read was unreadable, or part of a batched write was rejected after other elements succeeded. Output is not guaranteed; read the reported failures and reconcile before retrying. · `14` timed out — a bounded wait (`desktop wait --timeout`) ended before Zotero's connector answered; wait again. · `15` Zotero open but stuck — `desktop wait` found Zotero running past its startup window with a connector that does not answer or is off; tell the user. --- diff --git a/SKILL.md b/SKILL.md index e6631e77..d3ae2a5e 100644 --- a/SKILL.md +++ b/SKILL.md @@ -87,7 +87,7 @@ The curated feature set. `zotio which ""` resolves natural-language querie - **`export snapshot`** — Reproducible, resumable full-library JSONL export with a lockfile (key, version, content hash) — diff lockfiles to prove what changed between handoffs, and take one before any bulk write the journal cannot reverse. - **`watch`** — Periodic incremental syncs (`--interval`, `--once`); `--health` diffs library health between cycles and reports new findings to stdout or a webhook. - **`workflow run`** — Run a declarative multi-step spec (JSON) in-process with per-step status and continue-on-error — replaces brittle shell chains. -- **`desktop status` / `desktop wait`** — Is Zotero desktop running, and does its connector accept imports (`connector_reachable`)? `wait` blocks on filesystem events, not a poll, until the connector answers (exit 0), or exits 14 at `--timeout`. +- **`desktop status` / `desktop wait`** — Is Zotero desktop running, and does its connector accept imports (`connector_reachable`)? `wait` blocks on filesystem events, not a poll, until the connector answers (exit 0); it exits 15 when Zotero is open but `unresponsive` or `connector_off`, and 14 at `--timeout`. - **`init`** — Guided first run (detect Zotero, check the local API and explain how to enable it, set key, first sync, health check); agent-safe under `--no-input` (unmet steps exit 9 with a step report). ### Reading workflow @@ -289,6 +289,7 @@ Explicit flags always win over profile values; profile values win over defaults. | 12 | Stale data | | 13 | Incomplete — part succeeded, part was rejected; reconcile before retrying | | 14 | Timed out — `desktop wait --timeout` ended before Zotero's connector answered; wait again | +| 15 | Zotero is open but its connector is hung or off (`desktop wait`); tell the user rather than wait | ## Argument Parsing diff --git a/dev/zotero-api-coverage.md b/dev/zotero-api-coverage.md index fde6eea2..a98a1770 100644 --- a/dev/zotero-api-coverage.md +++ b/dev/zotero-api-coverage.md @@ -96,7 +96,11 @@ of coverage now. and delete-on-close instead. Only the connector answering proves an import can proceed; `desktop status` reports both signals and `desktop wait` sleeps on filesystem events for the profile and data directories until the connector - answers (`internal/desktop`). + answers (`internal/desktop`). Mozilla's lock open truncates `.parentlock`, so + its mtime is the lock time (measured: 3s after process start on a file created + years earlier); past a 2-minute startup window a silent connector is reported + as `unresponsive` (port accepts, no answer: seen live the same day, with the + window's accessibility tree also failing) or `connector_off` (refused). - **Schema/type endpoints are global**, served under `/api` directly, NOT under the `/users|groups/` library prefix the configured base URL carries: `/api/itemTypes`, `/api/itemFields`, `/api/itemTypeFields`, diff --git a/docs/reference/commands.md b/docs/reference/commands.md index 70a17bb3..de6ce602 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -742,16 +742,28 @@ Two signals, reported separately: profile lock (.parentlock via fcntl on macOS and Linux, parent.lock opened exclusively on Windows), or the connector answered. - connector_reachable GET /ping answered 200 during this check. - Imports and every other connector write need this. - -state is "ready" when the connector answers, "stopped" when neither signal -holds, and "starting" when the process holds its lock but the connector does -not answer. Zotero takes the lock about 3s after launch and its connector -listens a few seconds later, so "starting" is normal briefly after a launch; -it persists if the connector is disabled (Settings -> Advanced -> "Allow other -applications to communicate with Zotero"), moved to another port, or Zotero -is hung. evidence names the strongest signal: connector, profile_lock, none. + connector_reachable GET /ping answered 200 within 3s during this + check. Imports and every other connector write need + this. + +state: + ready the connector answers. + starting the lock is held, the connector does not answer yet, and the + lock is younger than the 2-minute startup window. Zotero + takes the lock about 3s after launch and its connector + listens a few seconds later. + unresponsive the lock is older than the startup window and the connector + port accepts the connection but does not answer (or answers + with an error): Zotero is open but not responding. + connector_off the lock is older than the startup window and nothing + listens on the connector port: the connector is disabled + (Settings -> Advanced -> "Allow other applications to + communicate with Zotero") or on another port. + stopped neither signal holds. + +The lock age comes from the lock file's modification time, which Zotero +resets when it takes the lock (profiles[].lock_since). evidence names the +strongest signal: connector, profile_lock, none. Profiles are discovered from profiles.ini in the platform's Zotero directory (ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js. @@ -776,11 +788,13 @@ status that proved it. If the connector already answers, it returns at once. While Zotero is closed nothing runs on a timer: the command sleeps on filesystem notifications for the Zotero profile and data directories and -probes only after a change. Once the profile lock is seen held, the connector -is re-checked on a capped backoff for up to 2 minutes, because it starts -listening a few seconds after the lock and its start writes no file. If Zotero -stays up with the connector silent after that, only filesystem changes cause -further checks. +probes only after a change. While Zotero is starting, the connector is +re-checked on a capped backoff (250ms up to 2s), because it listens a few +seconds after the lock and its start writes no file. When the startup window +passes without an answer, or Zotero is already past it (unresponsive or +connector_off), the command returns at once with exit 15 instead of waiting +silently: nothing on disk announces a recovery, so the caller tells the user +and decides when to wait again. Two signals, reported separately: @@ -788,19 +802,33 @@ Two signals, reported separately: profile lock (.parentlock via fcntl on macOS and Linux, parent.lock opened exclusively on Windows), or the connector answered. - connector_reachable GET /ping answered 200 during this check. - Imports and every other connector write need this. - -state is "ready" when the connector answers, "stopped" when neither signal -holds, and "starting" when the process holds its lock but the connector does -not answer. Zotero takes the lock about 3s after launch and its connector -listens a few seconds later, so "starting" is normal briefly after a launch; -it persists if the connector is disabled (Settings -> Advanced -> "Allow other -applications to communicate with Zotero"), moved to another port, or Zotero -is hung. evidence names the strongest signal: connector, profile_lock, none. + connector_reachable GET /ping answered 200 within 3s during this + check. Imports and every other connector write need + this. + +state: + ready the connector answers. + starting the lock is held, the connector does not answer yet, and the + lock is younger than the 2-minute startup window. Zotero + takes the lock about 3s after launch and its connector + listens a few seconds later. + unresponsive the lock is older than the startup window and the connector + port accepts the connection but does not answer (or answers + with an error): Zotero is open but not responding. + connector_off the lock is older than the startup window and nothing + listens on the connector port: the connector is disabled + (Settings -> Advanced -> "Allow other applications to + communicate with Zotero") or on another port. + stopped neither signal holds. + +The lock age comes from the lock file's modification time, which Zotero +resets when it takes the lock (profiles[].lock_since). evidence names the +strongest signal: connector, profile_lock, none. Exit codes and the JSON outcome field: 0 outcome "ready": the connector answers. + 15 outcome "unresponsive" or "connector_off" (the state): Zotero runs past + its startup window and its connector cannot take requests. 14 outcome "timeout": --timeout passed first; wait again. 9 outcome "no_profile" (no Zotero profile found and the connector does not answer) or "watch_failed" (the directories could not be watched). diff --git a/internal/cli/desktop.go b/internal/cli/desktop.go index 13a80227..aafc8b5f 100644 --- a/internal/cli/desktop.go +++ b/internal/cli/desktop.go @@ -49,16 +49,28 @@ const desktopRunningDefinition = `Two signals, reported separately: profile lock (.parentlock via fcntl on macOS and Linux, parent.lock opened exclusively on Windows), or the connector answered. - connector_reachable GET /ping answered 200 during this check. - Imports and every other connector write need this. + connector_reachable GET /ping answered 200 within 3s during this + check. Imports and every other connector write need + this. -state is "ready" when the connector answers, "stopped" when neither signal -holds, and "starting" when the process holds its lock but the connector does -not answer. Zotero takes the lock about 3s after launch and its connector -listens a few seconds later, so "starting" is normal briefly after a launch; -it persists if the connector is disabled (Settings -> Advanced -> "Allow other -applications to communicate with Zotero"), moved to another port, or Zotero -is hung. evidence names the strongest signal: connector, profile_lock, none.` +state: + ready the connector answers. + starting the lock is held, the connector does not answer yet, and the + lock is younger than the 2-minute startup window. Zotero + takes the lock about 3s after launch and its connector + listens a few seconds later. + unresponsive the lock is older than the startup window and the connector + port accepts the connection but does not answer (or answers + with an error): Zotero is open but not responding. + connector_off the lock is older than the startup window and nothing + listens on the connector port: the connector is disabled + (Settings -> Advanced -> "Allow other applications to + communicate with Zotero") or on another port. + stopped neither signal holds. + +The lock age comes from the lock file's modification time, which Zotero +resets when it takes the lock (profiles[].lock_since). evidence names the +strongest signal: connector, profile_lock, none.` func newDesktopCmd(flags *rootFlags) *cobra.Command { cmd := &cobra.Command{ @@ -114,16 +126,20 @@ status that proved it. If the connector already answers, it returns at once. While Zotero is closed nothing runs on a timer: the command sleeps on filesystem notifications for the Zotero profile and data directories and -probes only after a change. Once the profile lock is seen held, the connector -is re-checked on a capped backoff for up to 2 minutes, because it starts -listening a few seconds after the lock and its start writes no file. If Zotero -stays up with the connector silent after that, only filesystem changes cause -further checks. +probes only after a change. While Zotero is starting, the connector is +re-checked on a capped backoff (250ms up to 2s), because it listens a few +seconds after the lock and its start writes no file. When the startup window +passes without an answer, or Zotero is already past it (unresponsive or +connector_off), the command returns at once with exit 15 instead of waiting +silently: nothing on disk announces a recovery, so the caller tells the user +and decides when to wait again. ` + desktopRunningDefinition + ` Exit codes and the JSON outcome field: 0 outcome "ready": the connector answers. + 15 outcome "unresponsive" or "connector_off" (the state): Zotero runs past + its startup window and its connector cannot take requests. 14 outcome "timeout": --timeout passed first; wait again. 9 outcome "no_profile" (no Zotero profile found and the connector does not answer) or "watch_failed" (the directories could not be watched). @@ -178,7 +194,7 @@ ping is always bounded to 3s.`, } start := time.Now() - st, waitErr := desktop.Wait(ctx, desktop.WaitOptions{Prober: prober, WatchDirs: watchDirs}) + st, waitErr := desktopWait(ctx, desktop.WaitOptions{Prober: prober, WatchDirs: watchDirs}) result := desktopWaitResult{Status: st, WaitedMS: time.Since(start).Milliseconds()} var exitErr error @@ -188,6 +204,9 @@ ping is always bounded to 3s.`, case errors.Is(waitErr, errDesktopWaitTimeout): result.Outcome = desktopOutcomeTimeout exitErr = timeoutErr(fmt.Errorf("Zotero desktop's connector did not answer within %s (state %q)", timeout, st.State)) + case errors.Is(waitErr, desktop.ErrStuck): + result.Outcome = string(st.State) + exitErr = stuckErr(desktopStuckError(st)) case errors.Is(waitErr, desktop.ErrNoProfile): result.Outcome = desktopOutcomeNoProfile exitErr = preconditionErr(desktopNoProfileError(st)) @@ -219,6 +238,10 @@ ping is always bounded to 3s.`, return cmd } +// desktopWait is desktop.Wait; tests replace it to pin how each way a wait +// ends maps to an outcome and an exit code. +var desktopWait = desktop.Wait + // newDesktopProber discovers the installation and resolves the connector // from the configured base URL, the same resolution `import` uses. A base URL // that is not a local Zotero leaves no connector to ping; that is reported in @@ -241,6 +264,13 @@ func newDesktopProber(flags *rootFlags) (*desktop.Prober, error) { return prober, nil } +func desktopStuckError(st desktop.Status) error { + if st.State == desktop.StateConnectorOff { + return errors.New("Zotero desktop is running but nothing listens on its connector port; enable Settings -> Advanced -> \"Allow other applications to communicate with Zotero\"") + } + return errors.New("Zotero desktop is running but not responding: its connector accepts connections and does not answer") +} + func desktopNoProfileError(st desktop.Status) error { msg := "no Zotero desktop profile directory was found and the connector does not answer; install and start Zotero once, or set ZOTERO_PROFILE_DIR" if st.DiscoveryError != "" { @@ -255,6 +285,10 @@ func renderDesktopStatus(w io.Writer, st desktop.Status) { fmt.Fprintf(w, "Zotero desktop: ready (the connector answers at %s)\n", st.ConnectorURL) case desktop.StateStarting: fmt.Fprintln(w, "Zotero desktop: starting (the process holds its profile lock; the connector does not answer yet)") + case desktop.StateUnresponsive: + fmt.Fprintln(w, "Zotero desktop: unresponsive (open, but its connector accepts connections and does not answer)") + case desktop.StateConnectorOff: + fmt.Fprintln(w, "Zotero desktop: connector off (open, but nothing listens on the connector port)") default: fmt.Fprintln(w, "Zotero desktop: stopped") } @@ -264,8 +298,14 @@ func renderDesktopStatus(w io.Writer, st desktop.Status) { for _, p := range st.Profiles { lock := string(p.Lock) switch { - case p.Lock == desktop.LockHeld && p.LockPID > 0: - lock = fmt.Sprintf("held by pid %d", p.LockPID) + case p.Lock == desktop.LockHeld: + lock = "held" + if p.LockPID > 0 { + lock += fmt.Sprintf(" by pid %d", p.LockPID) + } + if p.LockSince != nil { + lock += " since " + p.LockSince.Local().Format(time.RFC3339) + } case p.LockError != "": lock = "error: " + p.LockError } diff --git a/internal/cli/desktop_test.go b/internal/cli/desktop_test.go index ab5f5782..0a9a4f62 100644 --- a/internal/cli/desktop_test.go +++ b/internal/cli/desktop_test.go @@ -15,6 +15,7 @@ import ( "time" "zotio/internal/connector" + "zotio/internal/desktop" "zotio/internal/zoteroprefs" ) @@ -157,6 +158,27 @@ func TestDesktopWaitExitCodesAndOutcomes(t *testing.T) { } }) + // Zotero open past its startup window with a connector that cannot take + // requests: the caller must learn that, not wait silently or be told to + // open Zotero. + for _, state := range []desktop.State{desktop.StateUnresponsive, desktop.StateConnectorOff} { + t.Run(string(state)+" exits 15", func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, false) + old := desktopWait + t.Cleanup(func() { desktopWait = old }) + desktopWait = func(context.Context, desktop.WaitOptions) (desktop.Status, error) { + return desktop.Status{Running: true, State: state, Evidence: desktop.EvidenceProfileLock, Profiles: []desktop.ProfileStatus{}}, desktop.ErrStuck + } + got, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "1h") + if code := ExitCode(err); code != 15 { + t.Fatalf("exit code = %d (err %v), want 15", code, err) + } + if got["outcome"] != string(state) || got["state"] != string(state) || got["running"] != true { + t.Fatalf("wait = %v, want outcome and state %q with running true", got, state) + } + }) + } + t.Run("no profile exits 9", func(t *testing.T) { flags, _ := desktopTestEnv(t, false, false) got, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "5s") @@ -225,3 +247,26 @@ func TestDesktopStatusHumanOutputNamesTheState(t *testing.T) { t.Fatal("status created a lock file in the profile") } } + +// A Zotero that is open but hung, or open with its connector off, must end +// the wait with its own outcome and exit code, so a caller tells the user +// "Zotero is open but not responding" instead of "open Zotero". +func TestDesktopWaitStuckExits15WithTheState(t *testing.T) { + for _, state := range []desktop.State{desktop.StateUnresponsive, desktop.StateConnectorOff} { + t.Run(string(state), func(t *testing.T) { + flags, _ := desktopTestEnv(t, true, false) + old := desktopWait + t.Cleanup(func() { desktopWait = old }) + desktopWait = func(context.Context, desktop.WaitOptions) (desktop.Status, error) { + return desktop.Status{Running: true, State: state, Evidence: desktop.EvidenceProfileLock, Profiles: []desktop.ProfileStatus{}}, desktop.ErrStuck + } + got, _, err := runDesktopCmd(t, flags, "", "wait", "--timeout", "1h") + if code := ExitCode(err); code != 15 { + t.Fatalf("exit code = %d (err %v), want 15", code, err) + } + if got["outcome"] != string(state) || got["state"] != string(state) || got["running"] != true { + t.Fatalf("wait = %v, want outcome and state %s", got, state) + } + }) + } +} diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index 7712072e..7f02d882 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -246,6 +246,13 @@ func degradedErr(err error) error { return &cliError{code: 13, err: err} } // environment first. func timeoutErr(err error) error { return &cliError{code: 14, err: err} } +// Zotero desktop is running past its startup window and its connector cannot +// take requests (`desktop wait`: unresponsive or connector_off). Unlike a +// timeout (14), waiting longer does not help until something changes, and +// unlike a missing precondition (9) the app is not closed: the remedy is to +// tell the user, whose Zotero is open but hung or has its connector off. +func stuckErr(err error) error { return &cliError{code: 15, err: err} } + // dryRunOK reports whether the command should short-circuit without doing any // real work because --dry-run was set. The verify pipeline probes hand-written // commands with --dry-run; commands that put validation in cobra's `Args:` or diff --git a/internal/desktop/lock_unix.go b/internal/desktop/lock_unix.go index accab50e..e444dd14 100644 --- a/internal/desktop/lock_unix.go +++ b/internal/desktop/lock_unix.go @@ -55,5 +55,6 @@ func probeLock(profileDir string) LockProbe { if lk.Type == syscall.F_UNLCK { return LockProbe{State: LockFree} } - return LockProbe{State: LockHeld, PID: int(lk.Pid)} + // The lock open truncates the file, so its mtime is the lock time. + return LockProbe{State: LockHeld, PID: int(lk.Pid), Since: info.ModTime()} } diff --git a/internal/desktop/lock_windows.go b/internal/desktop/lock_windows.go index 0a87026b..e5e927c6 100644 --- a/internal/desktop/lock_windows.go +++ b/internal/desktop/lock_windows.go @@ -7,8 +7,10 @@ package desktop import ( "errors" "fmt" + "os" "path/filepath" "syscall" + "time" ) // lockFileName is the file Mozilla's nsProfileLock opens on Windows with @@ -22,6 +24,13 @@ const errorSharingViolation syscall.Errno = 32 func probeLock(profileDir string) LockProbe { path := filepath.Join(profileDir, lockFileName) + // Attribute reads are not subject to share modes, so this works while + // Zotero holds the file. The file is created at each launch, so its + // mtime is the launch time. + var since time.Time + if info, err := os.Lstat(path); err == nil { + since = info.ModTime() + } name, err := syscall.UTF16PtrFromString(path) if err != nil { return LockProbe{State: LockError, Err: err} @@ -37,7 +46,7 @@ func probeLock(profileDir string) LockProbe { case errors.Is(err, syscall.ERROR_FILE_NOT_FOUND), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND): return LockProbe{State: LockAbsent} case errors.Is(err, errorSharingViolation): - return LockProbe{State: LockHeld} + return LockProbe{State: LockHeld, Since: since} default: return LockProbe{State: LockError, Err: fmt.Errorf("opening %s: %w", path, err)} } diff --git a/internal/desktop/presence.go b/internal/desktop/presence.go index 5728cc1e..a818a134 100644 --- a/internal/desktop/presence.go +++ b/internal/desktop/presence.go @@ -21,21 +21,36 @@ // is true only when the connector answered during this check, and it is the // field a caller that wants to import must gate on. // -// # The transition window +// # The transition window, and after it // // Measured against Zotero 7 on macOS, Zotero takes the profile lock about 3s // after launch and creates its database WAL files about 4s after launch; the // connector starts listening a few seconds later still. In between, the -// process is up and the connector refuses connections: Status.State is -// "starting". The same state persists if the connector is disabled, bound to -// another port, or Zotero is hung, so "starting" means "process up, connector -// not answering", not a promise that it will answer. +// process is up and the connector refuses connections or does not answer: +// Status.State is "starting". +// +// "starting" lasts only for the startup window (Prober.StartupWindow, 2 +// minutes by default) measured from when the lock was taken. Mozilla's lock +// open truncates the lock file, so its modification time is the lock time: +// measured on macOS, .parentlock's mtime was 3s after the Zotero process +// started, on a file created years earlier. On Windows parent.lock is +// deleted on exit and created again at launch, so its mtime is the launch +// time by construction. Past the window, a connector that still cannot take +// requests is reported as one of two states: +// +// - "unresponsive": the connector port accepts the connection but no +// answer arrives within the ping bound (or the answer is not a 200). +// Zotero's main thread is blocked: open, but not responding. +// - "connector_off": nothing listens on the connector port. The connector +// is disabled (Settings -> Advanced -> "Allow other applications to +// communicate with Zotero") or on another port. package desktop import ( "context" "errors" "fmt" + "net" "slices" "time" @@ -63,7 +78,11 @@ type LockProbe struct { // PID is the lock holder's process ID where the platform reports it // (fcntl F_GETLK on macOS and Linux), else 0. PID int - Err error + // Since is when the lock was taken: the lock file's modification time + // (see the package doc). Zero when the lock is not held or the time is + // unknown. + Since time.Time + Err error } // ProbeLock reports whether another process holds the Zotero profile lock in @@ -78,13 +97,25 @@ type State string const ( // StateReady means the connector answered: imports can proceed. StateReady State = "ready" - // StateStarting means the process holds its profile lock but the - // connector did not answer (see the package doc's transition window). + // StateStarting means the process holds its profile lock, the connector + // does not answer yet, and the lock is younger than the startup window. StateStarting State = "starting" + // StateUnresponsive means the lock is older than the startup window and + // the connector port accepts connections without answering: Zotero is + // open but not responding. + StateUnresponsive State = "unresponsive" + // StateConnectorOff means the lock is older than the startup window and + // nothing listens on the connector port. + StateConnectorOff State = "connector_off" // StateStopped means neither signal holds. StateStopped State = "stopped" ) +// Stuck reports whether Zotero is running past its startup window without a +// usable connector (StateUnresponsive or StateConnectorOff): waiting longer +// will not help until something changes, so a caller should tell the user. +func (s State) Stuck() bool { return s == StateUnresponsive || s == StateConnectorOff } + // Evidence names the strongest signal behind Status.Running. type Evidence string @@ -96,10 +127,11 @@ const ( // ProfileStatus is one profile directory and its lock. type ProfileStatus struct { - Path string `json:"path"` - Lock LockState `json:"lock"` - LockPID int `json:"lock_pid,omitempty"` - LockError string `json:"lock_error,omitempty"` + Path string `json:"path"` + Lock LockState `json:"lock"` + LockPID int `json:"lock_pid,omitempty"` + LockSince *time.Time `json:"lock_since,omitempty"` + LockError string `json:"lock_error,omitempty"` } // Status is one presence check. Its JSON shape is the machine contract of @@ -122,6 +154,19 @@ func (s Status) LockHeld() bool { return slices.ContainsFunc(s.Profiles, func(p ProfileStatus) bool { return p.Lock == LockHeld }) } +// lockSince is when the newest held lock was taken, zero if no held lock has +// a known time. The newest lock is the conservative choice: a profile locked +// moments ago is still starting whatever an older lock says. +func (s Status) lockSince() time.Time { + var newest time.Time + for _, p := range s.Profiles { + if p.Lock == LockHeld && p.LockSince != nil && p.LockSince.After(newest) { + newest = *p.LockSince + } + } + return newest +} + // Install is the Zotero desktop installation discovery found. type Install struct { // Profiles are the profile directories to probe, preferred first. @@ -180,6 +225,12 @@ var ErrConnectorUnavailable = errors.New("desktop connector unavailable") // refuses the connection at once. const DefaultPingTimeout = 3 * time.Second +// DefaultStartupWindow is how long after taking its profile lock a Zotero +// whose connector does not answer counts as "starting". The connector +// normally listens within seconds; a database upgrade after a Zotero update +// can take longer, and is reported as unresponsive once it outlasts this. +const DefaultStartupWindow = 2 * time.Minute + // Prober checks both presence signals. type Prober struct { Install Install @@ -190,11 +241,27 @@ type Prober struct { ConnectorErr error // PingTimeout bounds each ping; zero means DefaultPingTimeout. PingTimeout time.Duration + // StartupWindow bounds StateStarting; zero means DefaultStartupWindow. + StartupWindow time.Duration +} + +func (p *Prober) startupWindow() time.Duration { + if p.StartupWindow > 0 { + return p.StartupWindow + } + return DefaultStartupWindow } // Probe runs one presence check. It never fails: an undecidable signal is // reported in the Status rather than returned as an error. func (p *Prober) Probe(ctx context.Context) Status { + return p.probe(ctx, time.Time{}) +} + +// probe runs one check. firstSeen is when the caller first saw the lock held; +// it stands in for the lock time when the platform could not report one, so a +// long-held lock of unknown age still leaves "starting" eventually. +func (p *Prober) probe(ctx context.Context, firstSeen time.Time) Status { st := Status{ ConnectorURL: p.ConnectorURL, Profiles: make([]ProfileStatus, 0, len(p.Install.Profiles)), @@ -206,43 +273,85 @@ func (p *Prober) Probe(ctx context.Context) Status { for _, dir := range p.Install.Profiles { lp := ProbeLock(dir) ps := ProfileStatus{Path: dir, Lock: lp.State, LockPID: lp.PID} + if lp.State == LockHeld && !lp.Since.IsZero() { + since := lp.Since.UTC() + ps.LockSince = &since + } if lp.Err != nil { ps.LockError = lp.Err.Error() } st.Profiles = append(st.Profiles, ps) } + var pingErr error if p.Ping == nil { - err := p.ConnectorErr - if err == nil { - err = ErrConnectorUnavailable + pingErr = p.ConnectorErr + if pingErr == nil { + pingErr = ErrConnectorUnavailable } - st.ConnectorError = err.Error() } else { timeout := p.PingTimeout if timeout <= 0 { timeout = DefaultPingTimeout } pingCtx, cancel := context.WithTimeout(ctx, timeout) - err := p.Ping(pingCtx) + pingErr = p.Ping(pingCtx) cancel() - if err != nil { - st.ConnectorError = err.Error() - } else { - st.ConnectorReachable = true - } + } + if pingErr != nil { + st.ConnectorError = pingErr.Error() + } else { + st.ConnectorReachable = true } + now := time.Now() held := st.LockHeld() st.Running = held || st.ConnectorReachable switch { case st.ConnectorReachable: st.State, st.Evidence = StateReady, EvidenceConnector case held: - st.State, st.Evidence = StateStarting, EvidenceProfileLock + st.Evidence = EvidenceProfileLock + since := st.lockSince() + if since.IsZero() { + since = firstSeen + } + switch { + case since.IsZero() || now.Sub(since) < p.startupWindow(): + st.State = StateStarting + case p.Ping == nil || connectorRefused(pingErr): + // No connector zotio can address counts as none listening. + st.State = StateConnectorOff + default: + st.State = StateUnresponsive + } default: st.State, st.Evidence = StateStopped, EvidenceNone } - st.CheckedAt = time.Now().UTC() + st.CheckedAt = now.UTC() return st } + +// startingUntil is when a "starting" status stops being one. +func (p *Prober) startingUntil(st Status, firstSeen time.Time) time.Time { + since := st.lockSince() + if since.IsZero() { + since = firstSeen + } + return since.Add(p.startupWindow()) +} + +// connectorRefused reports whether a ping failed because nothing listens on +// the port: the dial itself failed. A timeout, including a dial timeout, is +// the opposite case: something holds the port and does not answer. +func connectorRefused(err error) bool { + if err == nil { + return false + } + var ne net.Error + if errors.Is(err, context.DeadlineExceeded) || (errors.As(err, &ne) && ne.Timeout()) { + return false + } + var op *net.OpError + return errors.As(err, &op) && op.Op == "dial" +} diff --git a/internal/desktop/presence_test.go b/internal/desktop/presence_test.go index 3c32854b..861014d4 100644 --- a/internal/desktop/presence_test.go +++ b/internal/desktop/presence_test.go @@ -5,6 +5,11 @@ package desktop import ( "context" "errors" + "fmt" + "net" + "net/http" + "net/http/httptest" + "net/url" "os" "path/filepath" "runtime" @@ -13,6 +18,7 @@ import ( "github.com/fsnotify/fsnotify" + "zotio/internal/connector" "zotio/internal/zoteroprefs" ) @@ -83,6 +89,9 @@ func TestProbeLockRaisesNoFilesystemEvents(t *testing.T) { func TestProbeStatesFollowTheTwoSignals(t *testing.T) { refused := func(context.Context) error { return errors.New("connection refused") } + noAnswer := func(context.Context) error { + return fmt.Errorf("connector ping: %w", &url.Error{Op: "Get", URL: "http://127.0.0.1:23119/connector/ping", Err: context.DeadlineExceeded}) + } answers := func(context.Context) error { return nil } t.Run("stopped", func(t *testing.T) { @@ -108,6 +117,49 @@ func TestProbeStatesFollowTheTwoSignals(t *testing.T) { if len(st.Profiles) != 1 || st.Profiles[0].Lock != LockHeld { t.Fatalf("profiles = %+v, want the one profile held", st.Profiles) } + info, err := os.Stat(filepath.Join(dir, lockFileName)) + if err != nil { + t.Fatal(err) + } + if got := st.Profiles[0].LockSince; got == nil || !got.Equal(info.ModTime().UTC()) { + t.Fatalf("lock_since = %v, want the lock file mtime %v", got, info.ModTime().UTC()) + } + }) + + // The live case, 2026-09-24: Zotero up for well over the startup window, + // its connector port accepting connections and never answering. + t.Run("unresponsive past the startup window", func(t *testing.T) { + dir := t.TempDir() + startLockHolder(t, dir) + for _, startup := range []time.Duration{time.Hour, time.Nanosecond} { + p := &Prober{Install: Install{Profiles: []string{dir}}, Ping: noAnswer, StartupWindow: startup} + st := p.Probe(t.Context()) + want := StateUnresponsive + if startup == time.Hour { + want = StateStarting // the same silence inside the window is a start + } + if !st.Running || st.State != want || st.Evidence != EvidenceProfileLock { + t.Fatalf("startup window %v: status = %+v, want running, %s", startup, st, want) + } + } + }) + + t.Run("connector_off past the startup window", func(t *testing.T) { + dir := t.TempDir() + startLockHolder(t, dir) + refusedDial := func(context.Context) error { return realRefusal(t) } + p := &Prober{Install: Install{Profiles: []string{dir}}, Ping: refusedDial, StartupWindow: time.Nanosecond} + if st := p.Probe(t.Context()); !st.Running || st.State != StateConnectorOff { + t.Fatalf("status = %+v, want running, connector_off", st) + } + }) + + // A closed Zotero is stopped however old its lock file is. + t.Run("stopped ignores the window", func(t *testing.T) { + p := &Prober{Install: Install{Profiles: []string{t.TempDir()}}, Ping: noAnswer, StartupWindow: time.Nanosecond} + if st := p.Probe(t.Context()); st.Running || st.State != StateStopped { + t.Fatalf("status = %+v, want stopped", st) + } }) // The connector is the stronger signal: it answers even when discovery @@ -176,3 +228,57 @@ func TestDiscoverReportsABadPinInsteadOfGuessing(t *testing.T) { t.Fatalf("Discover with a pin at a missing directory = %+v, want an error and no profiles", in) } } + +// realRefusal dials a port nothing listens on, as a ping to a closed or +// connector-disabled Zotero does. +func realRefusal(t *testing.T) error { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + addr := ln.Addr().String() + _ = ln.Close() + conn := connector.New("http://"+addr+"/connector", time.Second) + err = conn.Ping(t.Context()) + if err == nil { + t.Fatal("ping to a closed port succeeded") + } + return err +} + +// The unresponsive/connector_off split rests on telling "nothing listens" +// from "something holds the port and does not answer", so it is pinned +// against the errors the real connector client returns. +func TestConnectorRefusedClassifiesRealPingErrors(t *testing.T) { + if !connectorRefused(realRefusal(t)) { + t.Fatal("a refused dial did not classify as refused") + } + + release := make(chan struct{}) + hung := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { // accept the connection, never answer: a blocked main thread + case <-r.Context().Done(): + case <-release: + } + })) + defer hung.Close() + defer close(release) + ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond) + defer cancel() + err := connector.New(hung.URL+"/connector", time.Minute).Ping(ctx) + if err == nil || connectorRefused(err) { + t.Fatalf("hung connector ping error = %v; want a failure that is not a refusal", err) + } + err = connector.New(hung.URL+"/connector", 100*time.Millisecond).Ping(t.Context()) + if err == nil || connectorRefused(err) { + t.Fatalf("client-timeout ping error = %v; want a failure that is not a refusal", err) + } + + wrong := httptest.NewServer(http.NotFoundHandler()) + defer wrong.Close() + err = connector.New(wrong.URL+"/connector", time.Second).Ping(t.Context()) + if err == nil || connectorRefused(err) { + t.Fatalf("404 ping error = %v; want a failure that is not a refusal", err) + } +} diff --git a/internal/desktop/wait.go b/internal/desktop/wait.go index a71614bb..44cad09c 100644 --- a/internal/desktop/wait.go +++ b/internal/desktop/wait.go @@ -16,6 +16,13 @@ import ( // not answer either. var ErrNoProfile = errors.New("no Zotero desktop profile directory found") +// ErrStuck means Zotero is running past its startup window and its connector +// cannot take requests; the returned Status.State says which way +// (StateUnresponsive or StateConnectorOff). No filesystem change announces a +// recovery, so Wait returns instead of waiting silently: the caller tells the +// user and decides when to wait again. +var ErrStuck = errors.New("Zotero desktop is running but its connector cannot take requests") + // Wait tuning. A package-level default so a caller passing a zero WaitOptions // gets the documented behaviour. const ( @@ -23,28 +30,22 @@ const ( // a Zotero start touches the lock file and creates two WAL files within // about a second. DefaultSettle = 200 * time.Millisecond - // DefaultConfirmWindow bounds the connector re-checks after the profile - // lock is first seen held. Zotero's connector listens a few seconds after - // the lock; a database upgrade after a Zotero update can take longer, and - // such an upgrade keeps writing the WAL, which raises further events - // after the window closes. - DefaultConfirmWindow = 2 * time.Minute - // DefaultConfirmFirst and DefaultConfirmMax shape the confirm backoff: - // the first re-check comes quickly, later ones at most this far apart. + // DefaultConfirmFirst and DefaultConfirmMax shape the connector re-checks + // while Zotero is starting: the first comes quickly, later ones at most + // this far apart. DefaultConfirmFirst = 250 * time.Millisecond DefaultConfirmMax = 2 * time.Second ) -// WaitOptions configures Wait. +// WaitOptions configures Wait. The startup window is the Prober's. type WaitOptions struct { Prober *Prober // WatchDirs are watched for changes; see Install.WatchDirs. WatchDirs []string - Settle time.Duration - ConfirmWindow time.Duration - ConfirmFirst time.Duration - ConfirmMax time.Duration + Settle time.Duration + ConfirmFirst time.Duration + ConfirmMax time.Duration // OnWatching, when set, runs once the watches are installed and the // first probe found the connector down. Tests use it to know that a @@ -56,9 +57,6 @@ func (o WaitOptions) withDefaults() WaitOptions { if o.Settle <= 0 { o.Settle = DefaultSettle } - if o.ConfirmWindow <= 0 { - o.ConfirmWindow = DefaultConfirmWindow - } if o.ConfirmFirst <= 0 { o.ConfirmFirst = DefaultConfirmFirst } @@ -74,13 +72,12 @@ func (o WaitOptions) withDefaults() WaitOptions { // // While Zotero is closed nothing runs on a timer: Wait sleeps on filesystem // notifications for the profile and data directories and probes only after -// a change settles. Once the profile lock is seen held, the connector is -// re-checked on a capped backoff for ConfirmWindow, because the connector -// listens seconds after the lock and its start writes no file. If the window -// passes with the lock held and the connector still silent (connector -// disabled, another port, a hung Zotero), Wait goes back to sleeping on -// events; a new confirm window opens only when the lock is released and -// taken again. +// a change settles. While Zotero is starting (lock held, connector silent, +// lock younger than the startup window) the connector is re-checked on a +// capped backoff, because it listens seconds after the lock and its start +// writes no file. When the startup window passes with the connector still +// silent, or when Zotero is already past it, Wait returns ErrStuck with the +// unresponsive or connector_off Status. // // On cancellation Wait returns the last Status and context.Cause(ctx), so a // caller that set a deadline with context.WithTimeoutCause can tell a @@ -98,9 +95,33 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { defer watcher.Close() } - st := o.Prober.Probe(ctx) - if st.ConnectorReachable { - return st, nil + // firstSeen stands in for the lock time when the platform reports none. + var firstSeen time.Time + probe := func() Status { + st := o.Prober.probe(ctx, firstSeen) + switch { + case !st.LockHeld(): + firstSeen = time.Time{} + case firstSeen.IsZero(): + firstSeen = st.CheckedAt + } + return st + } + // settled reports whether st ends the wait, and how. + settled := func(st Status) (bool, error) { + switch { + case st.ConnectorReachable: + return true, nil + case st.State.Stuck(): + return true, ErrStuck + default: + return false, nil + } + } + + st := probe() + if done, err := settled(st); done { + return st, err } if watcher == nil { if watchErr == nil { @@ -113,11 +134,9 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { } var ( - settle *time.Timer - confirm *time.Timer - confirmDeadline time.Time - backoff time.Duration - lockSeen = st.LockHeld() + settle *time.Timer + confirm *time.Timer + backoff time.Duration ) stopTimer := func(t **time.Timer) { if *t != nil { @@ -127,26 +146,34 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { } defer stopTimer(&settle) defer stopTimer(&confirm) - startConfirm := func() { - stopTimer(&confirm) - confirmDeadline = time.Now().Add(o.ConfirmWindow) - backoff = o.ConfirmFirst - confirm = time.NewTimer(backoff) - } - if lockSeen { - startConfirm() - } timerC := func(t *time.Timer) <-chan time.Time { if t == nil { return nil } return t.C } - armSettle := func() { - if settle == nil { - settle = time.NewTimer(o.Settle) + // nextConfirm schedules the next re-check: the backoff step, but never + // past the end of the startup window, so leaving "starting" is noticed + // when it happens rather than up to one step later. + nextConfirm := func() { + delay := backoff + if until := time.Until(o.Prober.startingUntil(st, firstSeen)); until < delay { + delay = max(until, 0) + 10*time.Millisecond + } + confirm = time.NewTimer(delay) + } + // track keeps the re-checks running exactly while Zotero is starting. + track := func() { + if st.State != StateStarting { + stopTimer(&confirm) + return + } + if confirm == nil { + backoff = o.ConfirmFirst + nextConfirm() } } + track() for { select { @@ -157,7 +184,9 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { if !ok { return st, errors.New("filesystem watcher closed") } - armSettle() + if settle == nil { + settle = time.NewTimer(o.Settle) + } case _, ok := <-watcher.Errors: if !ok { @@ -165,36 +194,28 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { } // An overflow or read error means events may have been lost: // probe as though one arrived rather than trusting the silence. - armSettle() + if settle == nil { + settle = time.NewTimer(o.Settle) + } case <-timerC(settle): settle = nil - st = o.Prober.Probe(ctx) - if st.ConnectorReachable { - return st, nil - } - held := st.LockHeld() - switch { - case held && !lockSeen: - startConfirm() - case !held: - stopTimer(&confirm) + st = probe() + if done, err := settled(st); done { + return st, err } - lockSeen = held + track() case <-timerC(confirm): confirm = nil - st = o.Prober.Probe(ctx) - if st.ConnectorReachable { - return st, nil + st = probe() + if done, err := settled(st); done { + return st, err } - lockSeen = st.LockHeld() - remaining := time.Until(confirmDeadline) - if !lockSeen || remaining <= 0 { - continue + if st.State == StateStarting { + backoff = min(backoff*2, o.ConfirmMax) + nextConfirm() } - backoff = min(backoff*2, o.ConfirmMax) - confirm = time.NewTimer(min(backoff, remaining)) } } } diff --git a/internal/desktop/wait_test.go b/internal/desktop/wait_test.go index 1305bc33..9ba03de8 100644 --- a/internal/desktop/wait_test.go +++ b/internal/desktop/wait_test.go @@ -5,6 +5,8 @@ package desktop import ( "context" "errors" + "fmt" + "net/url" "os" "path/filepath" "sync" @@ -18,6 +20,8 @@ type fakeConnector struct { mu sync.Mutex up bool calls int + // fail is the error while down; nil means a generic failure. + fail error } func (f *fakeConnector) ping(context.Context) error { @@ -27,9 +31,16 @@ func (f *fakeConnector) ping(context.Context) error { if f.up { return nil } + if f.fail != nil { + return f.fail + } return errors.New("dial tcp 127.0.0.1:23119: connect: connection refused") } +// errNoAnswer is how a ping to a connector that accepts the connection and +// never answers fails. +var errNoAnswer = fmt.Errorf("connector ping: %w", &url.Error{Op: "Get", URL: "http://127.0.0.1:23119/connector/ping", Err: context.DeadlineExceeded}) + func (f *fakeConnector) setUp() { f.mu.Lock() f.up = true @@ -163,7 +174,7 @@ func TestWaitConfirmsTheConnectorAfterTheLockWithoutAFurtherEvent(t *testing.T) in := newInstall(t) conn := &fakeConnector{} done := startWait(t, t.Context(), in, conn, func(o *WaitOptions) { - o.ConfirmFirst, o.ConfirmMax, o.ConfirmWindow = 10*time.Millisecond, 40*time.Millisecond, 10*time.Second + o.ConfirmFirst, o.ConfirmMax = 10*time.Millisecond, 40*time.Millisecond }) h := startLockHolder(t, in.profile) // Zotero takes its profile lock @@ -183,42 +194,60 @@ func TestWaitConfirmsTheConnectorAfterTheLockWithoutAFurtherEvent(t *testing.T) h.release(t) } -// Zotero running with its connector disabled keeps the lock held and never -// answers. The confirm re-checks must stop when the window closes; after -// that only a filesystem event may cause a probe. -func TestWaitStopsReCheckingWhenTheConfirmWindowCloses(t *testing.T) { +// The live case: Zotero starts, takes its lock, and hangs before its +// connector answers. Once the startup window passes, Wait must return the +// unresponsive status rather than wait silently; no filesystem change would +// ever announce a recovery. +func TestWaitReturnsStuckWhenTheStartupWindowPassesWithoutAnAnswer(t *testing.T) { in := newInstall(t) - startLockHolder(t, in.profile) - conn := &fakeConnector{} - ctx, cancel := context.WithCancel(t.Context()) - defer cancel() - done := startWait(t, ctx, in, conn, func(o *WaitOptions) { - o.ConfirmFirst, o.ConfirmMax, o.ConfirmWindow = 10*time.Millisecond, 20*time.Millisecond, 150*time.Millisecond + conn := &fakeConnector{fail: errNoAnswer} + done := startWait(t, t.Context(), in, conn, func(o *WaitOptions) { + o.ConfirmFirst, o.ConfirmMax = 10*time.Millisecond, 40*time.Millisecond + o.Prober.StartupWindow = 400 * time.Millisecond }) - time.Sleep(400 * time.Millisecond) // well past the window - settled := conn.count() - if settled < 3 { - t.Fatalf("pings during the confirm window = %d, want re-checks", settled) + locked := time.Now() + startLockHolder(t, in.profile) + r := awaitResult(t, done) + if !errors.Is(r.err, ErrStuck) || r.st.State != StateUnresponsive || !r.st.Running { + t.Fatalf("Wait = %+v, %v; want ErrStuck with the unresponsive status", r.st, r.err) } - assertStillWaiting(t, done, 400*time.Millisecond) - if got := conn.count(); got != settled { - t.Fatalf("pings after the confirm window = %d, then %d; want no polling", settled, got) + // Not before the window: a slow start is still a start. + if elapsed := time.Since(locked); elapsed < 300*time.Millisecond { + t.Fatalf("Wait gave up %v after the lock, inside the 400ms startup window", elapsed) } - - if err := os.WriteFile(filepath.Join(in.data, "zotero.sqlite-wal"), []byte("wal"), 0o600); err != nil { - t.Fatal(err) + if conn.count() < 3 { + t.Fatalf("pings = %d, want re-checks during the startup window", conn.count()) } - deadline := time.Now().Add(5 * time.Second) - for conn.count() == settled { - if time.Now().After(deadline) { - t.Fatal("a filesystem event after the window caused no probe") - } - time.Sleep(5 * time.Millisecond) +} + +// Zotero already hung (or running with its connector disabled) when the +// wait begins: the answer is immediate, and names which of the two it is. +func TestWaitReturnsStuckAtOnceWhenZoteroIsAlreadyPastItsStart(t *testing.T) { + cases := []struct { + name string + fail func(t *testing.T) error + want State + }{ + {"hung", func(*testing.T) error { return errNoAnswer }, StateUnresponsive}, + {"connector disabled", realRefusal, StateConnectorOff}, } - cancel() - if r := awaitResult(t, done); !errors.Is(r.err, context.Canceled) { - t.Fatalf("Wait after cancel = %+v, %v; want context.Canceled", r.st, r.err) + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + in := newInstall(t) + startLockHolder(t, in.profile) + conn := &fakeConnector{fail: tc.fail(t)} + prober := &Prober{Install: Install{Profiles: []string{in.profile}, DataDir: in.data}, Ping: conn.ping, StartupWindow: time.Nanosecond} + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + st, err := Wait(ctx, WaitOptions{Prober: prober, WatchDirs: []string{in.profile, in.data}}) + if !errors.Is(err, ErrStuck) || st.State != tc.want { + t.Fatalf("Wait = %+v, %v; want ErrStuck in state %s", st, err, tc.want) + } + if conn.count() != 1 { + t.Fatalf("pings = %d, want 1: the first probe already decides", conn.count()) + } + }) } } diff --git a/internal/mcp/testdata/surface_mirror.golden.json b/internal/mcp/testdata/surface_mirror.golden.json index f1ba4592..635b1e40 100644 --- a/internal/mcp/testdata/surface_mirror.golden.json +++ b/internal/mcp/testdata/surface_mirror.golden.json @@ -709,7 +709,7 @@ }, { "name": "desktop_status", - "description": "Report whether Zotero desktop is running and whether its connector accepts\nrequests. Cheap and local: it reads the profile lock of every discovered\nZotero profile and sends one ping to the local connector. It exits 0 whatever\nit finds; read the fields, not the exit code.\n\nTwo signals, reported separately:\n\n running Zotero's process is up: another process holds the\n profile lock (.parentlock via fcntl on macOS and Linux,\n parent.lock opened exclusively on Windows), or the\n connector answered.\n connector_reachable GET \u003cconnector\u003e/ping answered 200 during this check.\n Imports and every other connector write need this.\n\nstate is \"ready\" when the connector answers, \"stopped\" when neither signal\nholds, and \"starting\" when the process holds its lock but the connector does\nnot answer. Zotero takes the lock about 3s after launch and its connector\nlistens a few seconds later, so \"starting\" is normal briefly after a launch;\nit persists if the connector is disabled (Settings -\u003e Advanced -\u003e \"Allow other\napplications to communicate with Zotero\"), moved to another port, or Zotero\nis hung. evidence names the strongest signal: connector, profile_lock, none.\n\nProfiles are discovered from profiles.ini in the platform's Zotero directory\n(ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js.", + "description": "Report whether Zotero desktop is running and whether its connector accepts\nrequests. Cheap and local: it reads the profile lock of every discovered\nZotero profile and sends one ping to the local connector. It exits 0 whatever\nit finds; read the fields, not the exit code.\n\nTwo signals, reported separately:\n\n running Zotero's process is up: another process holds the\n profile lock (.parentlock via fcntl on macOS and Linux,\n parent.lock opened exclusively on Windows), or the\n connector answered.\n connector_reachable GET \u003cconnector\u003e/ping answered 200 within 3s during this\n check. Imports and every other connector write need\n this.\n\nstate:\n ready the connector answers.\n starting the lock is held, the connector does not answer yet, and the\n lock is younger than the 2-minute startup window. Zotero\n takes the lock about 3s after launch and its connector\n listens a few seconds later.\n unresponsive the lock is older than the startup window and the connector\n port accepts the connection but does not answer (or answers\n with an error): Zotero is open but not responding.\n connector_off the lock is older than the startup window and nothing\n listens on the connector port: the connector is disabled\n (Settings -\u003e Advanced -\u003e \"Allow other applications to\n communicate with Zotero\") or on another port.\n stopped neither signal holds.\n\nThe lock age comes from the lock file's modification time, which Zotero\nresets when it takes the lock (profiles[].lock_since). evidence names the\nstrongest signal: connector, profile_lock, none.\n\nProfiles are discovered from profiles.ini in the platform's Zotero directory\n(ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js.", "inputSchema": { "properties": {}, "required": [], From 9cb09edd55a213a04290f770524f9e04a4be1c75 Mon Sep 17 00:00:00 2001 From: enieuwy <121954036+enieuwy@users.noreply.github.com> Date: Thu, 24 Sep 2026 16:29:29 +0800 Subject: [PATCH 3/5] fix(desktop): require a sustained stall before calling Zotero unresponsive One 3s ping timeout past the startup window was enough to report "unresponsive", but Zotero's connector runs on its main thread, which a large sync can hold for seconds; papio would then tell the user to restart a Zotero that was only busy. A one-shot check past the window now reports "busy" for a connector that accepts and does not answer. "unresponsive" is reported only by desktop wait, after the connector stayed silent for at least 60s across 3 or more checks (re-checked every 20s with a 10s ping); any answer in between ends the wait as ready. Wait results carry stalled_since. Refusal is weak evidence too. Measured against the hung Zotero: it listened on 127.0.0.1 only, [::1] refused, and Go reports the first address's error, so the ping said "connection refused" for a held port; 127.0.0.1 also refused some back-to-back connects. connector_off now needs every resolved address to refuse repeated dials (desktop.ListeningOn), and in wait the whole stall span; a stall in which any check found a listener is unresponsive. While starting or busy, the re-check timer alone drives probes, so a sync's constant WAL writes no longer turn into extra pings. Closed Zotero still sleeps on filesystem events with no polling. --- CHANGELOG.md | 19 +- SKILL.md | 2 +- dev/zotero-api-coverage.md | 12 +- docs/reference/commands.md | 48 +++-- internal/cli/desktop.go | 33 ++-- internal/cli/helpers.go | 3 +- internal/desktop/presence.go | 137 ++++++++++++-- internal/desktop/presence_test.go | 116 +++++++++++- internal/desktop/wait.go | 170 +++++++++++++----- internal/desktop/wait_test.go | 145 +++++++++++---- .../mcp/testdata/surface_mirror.golden.json | 2 +- 11 files changed, 550 insertions(+), 137 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 92c1cc97..bd65a5bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,18 +13,25 @@ Notable changes to zotio. Format follows [Keep a Changelog](https://keepachangel process is up (lock held, or the connector answered); `connector_reachable` means the connector accepts requests now, which imports need. `state` is `ready`; `starting` (lock held, connector silent, lock younger than the - 2-minute startup window); `unresponsive` (past the window, the connector - port accepts connections but does not answer: Zotero is open but hung); - `connector_off` (past the window, nothing listens on the connector port); - or `stopped`. The lock age is the lock file's modification time, which + 2-minute startup window); `busy` (past the window, the connector port + accepted the connection but did not answer this check — one silent check + is not a hang, since a large sync can hold Zotero's main thread for + seconds); `connector_off` (past the window, every address of the + connector host refused repeated connects — a hung Zotero's listener can + refuse some, so one refused ping is not enough); or `stopped`. `unresponsive` is reported only by + `desktop wait`, after a sustained stall. The lock age is the lock file's modification time, which Zotero resets when it takes the lock (`profiles[].lock_since`). It exits 0 whatever it finds. - **`zotio desktop wait` blocks until Zotero's connector accepts requests.** It returns at once if the connector already answers. While Zotero is closed it sleeps on filesystem notifications for the profile and data directories instead of polling, and probes only after a change; while Zotero is starting - it re-checks the connector on a capped backoff. When Zotero is, or becomes, - `unresponsive` or `connector_off`, it returns at once with exit 15 and that + it re-checks the connector on a capped backoff. Past the startup window a + connector that cannot take a request is re-checked every 20s with a 10s + ping; only 60s without an answer, across 3 or more checks, returns exit 15: + `unresponsive` if any check found a listener on the connector port, + `connector_off` if none did (`stalled_since` set). Any answer in between + ends the wait as ready. Exit 15 carries the state as `outcome`, because nothing on disk announces a recovery. `--timeout` bounds the wait (exit 14, `outcome: "timeout"`); no discoverable profile exits 9 (`outcome: "no_profile"`); `--watch-stdin` diff --git a/SKILL.md b/SKILL.md index d3ae2a5e..440bcd74 100644 --- a/SKILL.md +++ b/SKILL.md @@ -87,7 +87,7 @@ The curated feature set. `zotio which ""` resolves natural-language querie - **`export snapshot`** — Reproducible, resumable full-library JSONL export with a lockfile (key, version, content hash) — diff lockfiles to prove what changed between handoffs, and take one before any bulk write the journal cannot reverse. - **`watch`** — Periodic incremental syncs (`--interval`, `--once`); `--health` diffs library health between cycles and reports new findings to stdout or a webhook. - **`workflow run`** — Run a declarative multi-step spec (JSON) in-process with per-step status and continue-on-error — replaces brittle shell chains. -- **`desktop status` / `desktop wait`** — Is Zotero desktop running, and does its connector accept imports (`connector_reachable`)? `wait` blocks on filesystem events, not a poll, until the connector answers (exit 0); it exits 15 when Zotero is open but `unresponsive` or `connector_off`, and 14 at `--timeout`. +- **`desktop status` / `desktop wait`** — Is Zotero desktop running, and does its connector accept imports (`connector_reachable`)? `wait` blocks on filesystem events, not a poll, until the connector answers (exit 0); it exits 15 when Zotero is open and its connector has not answered for 60s (`unresponsive`, or `connector_off` if nothing listens); a single silent check is only `busy`, and 14 at `--timeout`. - **`init`** — Guided first run (detect Zotero, check the local API and explain how to enable it, set key, first sync, health check); agent-safe under `--no-input` (unmet steps exit 9 with a step report). ### Reading workflow diff --git a/dev/zotero-api-coverage.md b/dev/zotero-api-coverage.md index a98a1770..5373ece6 100644 --- a/dev/zotero-api-coverage.md +++ b/dev/zotero-api-coverage.md @@ -99,8 +99,16 @@ of coverage now. answers (`internal/desktop`). Mozilla's lock open truncates `.parentlock`, so its mtime is the lock time (measured: 3s after process start on a file created years earlier); past a 2-minute startup window a silent connector is reported - as `unresponsive` (port accepts, no answer: seen live the same day, with the - window's accessibility tree also failing) or `connector_off` (refused). + as `busy` for one check, `unresponsive` only after 60s of silence across 3+ + checks (seen live the same day, with the window's accessibility tree also + failing), or `connector_off` (refused). The connector runs on Zotero's main + thread, so a long sync can stall it for seconds without a hang. + The hung listener is also why refusal is weak evidence: Zotero listened on + 127.0.0.1 only, `[::1]` refused, and Go reports the first address's error, so + a ping said "connection refused" for a held port; and 127.0.0.1 itself reset + or refused some back-to-back connects. `connector_off` therefore needs every + address to refuse repeated dials (`desktop.ListeningOn`), and in `wait` the + whole stall span. - **Schema/type endpoints are global**, served under `/api` directly, NOT under the `/users|groups/` library prefix the configured base URL carries: `/api/itemTypes`, `/api/itemFields`, `/api/itemTypeFields`, diff --git a/docs/reference/commands.md b/docs/reference/commands.md index de6ce602..c33fe0db 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -752,13 +752,19 @@ state: lock is younger than the 2-minute startup window. Zotero takes the lock about 3s after launch and its connector listens a few seconds later. - unresponsive the lock is older than the startup window and the connector - port accepts the connection but does not answer (or answers - with an error): Zotero is open but not responding. + busy the lock is older than the startup window and the connector + port accepted the connection but did not answer in this + check (or answered with an error). One silent check is not + a hang: a large sync can hold Zotero's main thread for + seconds. + unresponsive desktop wait only: the connector stayed silent for at least + 60s across 3 or more checks (each allowed 10s): Zotero is + open but not responding. connector_off the lock is older than the startup window and nothing - listens on the connector port: the connector is disabled - (Settings -> Advanced -> "Allow other applications to - communicate with Zotero") or on another port. + listens on the connector port (every address refused + repeated connects): the connector is disabled (Settings -> + Advanced -> "Allow other applications to communicate with + Zotero") or on another port. stopped neither signal holds. The lock age comes from the lock file's modification time, which Zotero @@ -791,10 +797,14 @@ filesystem notifications for the Zotero profile and data directories and probes only after a change. While Zotero is starting, the connector is re-checked on a capped backoff (250ms up to 2s), because it listens a few seconds after the lock and its start writes no file. When the startup window -passes without an answer, or Zotero is already past it (unresponsive or -connector_off), the command returns at once with exit 15 instead of waiting -silently: nothing on disk announces a recovery, so the caller tells the user -and decides when to wait again. +passes, a connector that cannot take a request is re-checked every 20s with a +10s ping. Any answer ends the wait as ready. 60s without one, across 3 or more +checks, ends it with exit 15: unresponsive if any check found a listener on +the connector port, connector_off if none did (a hung Zotero's listener also +refuses some connects, so one refusal proves nothing). Exit 15 means Zotero is open +but stuck: nothing on disk announces a recovery, so the caller tells the user +and decides when to wait again. While Zotero is starting or busy, filesystem +events cause no extra checks. Two signals, reported separately: @@ -812,13 +822,19 @@ state: lock is younger than the 2-minute startup window. Zotero takes the lock about 3s after launch and its connector listens a few seconds later. - unresponsive the lock is older than the startup window and the connector - port accepts the connection but does not answer (or answers - with an error): Zotero is open but not responding. + busy the lock is older than the startup window and the connector + port accepted the connection but did not answer in this + check (or answered with an error). One silent check is not + a hang: a large sync can hold Zotero's main thread for + seconds. + unresponsive desktop wait only: the connector stayed silent for at least + 60s across 3 or more checks (each allowed 10s): Zotero is + open but not responding. connector_off the lock is older than the startup window and nothing - listens on the connector port: the connector is disabled - (Settings -> Advanced -> "Allow other applications to - communicate with Zotero") or on another port. + listens on the connector port (every address refused + repeated connects): the connector is disabled (Settings -> + Advanced -> "Allow other applications to communicate with + Zotero") or on another port. stopped neither signal holds. The lock age comes from the lock file's modification time, which Zotero diff --git a/internal/cli/desktop.go b/internal/cli/desktop.go index aafc8b5f..39bf4260 100644 --- a/internal/cli/desktop.go +++ b/internal/cli/desktop.go @@ -59,13 +59,19 @@ state: lock is younger than the 2-minute startup window. Zotero takes the lock about 3s after launch and its connector listens a few seconds later. - unresponsive the lock is older than the startup window and the connector - port accepts the connection but does not answer (or answers - with an error): Zotero is open but not responding. + busy the lock is older than the startup window and the connector + port accepted the connection but did not answer in this + check (or answered with an error). One silent check is not + a hang: a large sync can hold Zotero's main thread for + seconds. + unresponsive desktop wait only: the connector stayed silent for at least + 60s across 3 or more checks (each allowed 10s): Zotero is + open but not responding. connector_off the lock is older than the startup window and nothing - listens on the connector port: the connector is disabled - (Settings -> Advanced -> "Allow other applications to - communicate with Zotero") or on another port. + listens on the connector port (every address refused + repeated connects): the connector is disabled (Settings -> + Advanced -> "Allow other applications to communicate with + Zotero") or on another port. stopped neither signal holds. The lock age comes from the lock file's modification time, which Zotero @@ -129,10 +135,14 @@ filesystem notifications for the Zotero profile and data directories and probes only after a change. While Zotero is starting, the connector is re-checked on a capped backoff (250ms up to 2s), because it listens a few seconds after the lock and its start writes no file. When the startup window -passes without an answer, or Zotero is already past it (unresponsive or -connector_off), the command returns at once with exit 15 instead of waiting -silently: nothing on disk announces a recovery, so the caller tells the user -and decides when to wait again. +passes, a connector that cannot take a request is re-checked every 20s with a +10s ping. Any answer ends the wait as ready. 60s without one, across 3 or more +checks, ends it with exit 15: unresponsive if any check found a listener on +the connector port, connector_off if none did (a hung Zotero's listener also +refuses some connects, so one refusal proves nothing). Exit 15 means Zotero is open +but stuck: nothing on disk announces a recovery, so the caller tells the user +and decides when to wait again. While Zotero is starting or busy, filesystem +events cause no extra checks. ` + desktopRunningDefinition + ` @@ -261,6 +271,7 @@ func newDesktopProber(flags *rootFlags) (*desktop.Prober, error) { conn := connector.New(base, desktop.DefaultPingTimeout) prober.ConnectorURL = base prober.Ping = func(ctx context.Context) error { return connectorPing(ctx, conn) } + prober.Listening = func(ctx context.Context) bool { return desktop.ListeningOn(ctx, base) } return prober, nil } @@ -287,6 +298,8 @@ func renderDesktopStatus(w io.Writer, st desktop.Status) { fmt.Fprintln(w, "Zotero desktop: starting (the process holds its profile lock; the connector does not answer yet)") case desktop.StateUnresponsive: fmt.Fprintln(w, "Zotero desktop: unresponsive (open, but its connector accepts connections and does not answer)") + case desktop.StateBusy: + fmt.Fprintln(w, "Zotero desktop: busy (open; its connector accepted the connection but did not answer this check)") case desktop.StateConnectorOff: fmt.Fprintln(w, "Zotero desktop: connector off (open, but nothing listens on the connector port)") default: diff --git a/internal/cli/helpers.go b/internal/cli/helpers.go index 7f02d882..b3a308eb 100644 --- a/internal/cli/helpers.go +++ b/internal/cli/helpers.go @@ -247,7 +247,8 @@ func degradedErr(err error) error { return &cliError{code: 13, err: err} } func timeoutErr(err error) error { return &cliError{code: 14, err: err} } // Zotero desktop is running past its startup window and its connector cannot -// take requests (`desktop wait`: unresponsive or connector_off). Unlike a +// take requests (`desktop wait`: unresponsive after a sustained stall, or +// connector_off). Unlike a // timeout (14), waiting longer does not help until something changes, and // unlike a missing precondition (9) the app is not closed: the remedy is to // tell the user, whose Zotero is open but hung or has its connector off. diff --git a/internal/desktop/presence.go b/internal/desktop/presence.go index a818a134..3586d0ea 100644 --- a/internal/desktop/presence.go +++ b/internal/desktop/presence.go @@ -36,14 +36,23 @@ // started, on a file created years earlier. On Windows parent.lock is // deleted on exit and created again at launch, so its mtime is the launch // time by construction. Past the window, a connector that still cannot take -// requests is reported as one of two states: +// requests is reported as: // -// - "unresponsive": the connector port accepts the connection but no -// answer arrives within the ping bound (or the answer is not a 200). -// Zotero's main thread is blocked: open, but not responding. +// - "busy": the connector port accepts the connection but no answer +// arrived within this check's ping bound (or the answer was not a 200). +// One silent check is not evidence of a hang: Zotero's connector runs on +// its main thread, which a large sync or database operation can hold for +// seconds. A one-shot Probe never goes further than "busy". +// - "unresponsive": only Wait reports it, after the connector has stayed +// silent for a sustained stall (StallChecks failed pings, each bounded by +// StallPingTimeout, spanning at least StallSpan). Zotero is open but not +// responding. // - "connector_off": nothing listens on the connector port. The connector // is disabled (Settings -> Advanced -> "Allow other applications to -// communicate with Zotero") or on another port. +// communicate with Zotero") or on another port. A hung Zotero's listener +// can also refuse some connects, so a one-shot Probe reports it only +// when every address refuses repeated dials, and Wait only when that +// held on every check of a sustained stall. package desktop import ( @@ -51,6 +60,7 @@ import ( "errors" "fmt" "net" + "net/url" "slices" "time" @@ -100,9 +110,14 @@ const ( // StateStarting means the process holds its profile lock, the connector // does not answer yet, and the lock is younger than the startup window. StateStarting State = "starting" - // StateUnresponsive means the lock is older than the startup window and - // the connector port accepts connections without answering: Zotero is - // open but not responding. + // StateBusy means the lock is older than the startup window and the + // connector port accepted the connection without answering in this + // check. It may be a brief stall (a large sync) or the start of a hang; + // Wait tells the two apart. + StateBusy State = "busy" + // StateUnresponsive means the connector stayed silent through a + // sustained stall (see StallSpan): Zotero is open but not responding. + // Only Wait reports it. StateUnresponsive State = "unresponsive" // StateConnectorOff means the lock is older than the startup window and // nothing listens on the connector port. @@ -146,7 +161,10 @@ type Status struct { Profiles []ProfileStatus `json:"profiles"` DiscoveryError string `json:"discovery_error,omitempty"` DataDir string `json:"data_dir,omitempty"` - CheckedAt time.Time `json:"checked_at"` + // StalledSince is when Wait first saw the connector silent past the + // startup window, set on busy and unresponsive results from Wait. + StalledSince *time.Time `json:"stalled_since,omitempty"` + CheckedAt time.Time `json:"checked_at"` } // LockHeld reports whether any profile lock was held. @@ -228,9 +246,22 @@ const DefaultPingTimeout = 3 * time.Second // DefaultStartupWindow is how long after taking its profile lock a Zotero // whose connector does not answer counts as "starting". The connector // normally listens within seconds; a database upgrade after a Zotero update -// can take longer, and is reported as unresponsive once it outlasts this. +// can take longer, and is reported as busy once it outlasts this. const DefaultStartupWindow = 2 * time.Minute +// Sustained-stall evidence Wait needs before it reports StateUnresponsive: +// at least DefaultStallChecks failed pings, each allowed +// DefaultStallPingTimeout, the first and last at least DefaultStallSpan +// apart, with a re-check every DefaultStallRecheck in between. A sync that +// holds Zotero's main thread for tens of seconds and then answers never +// meets it. +const ( + DefaultStallSpan = time.Minute + DefaultStallChecks = 3 + DefaultStallRecheck = 20 * time.Second + DefaultStallPingTimeout = 10 * time.Second +) + // Prober checks both presence signals. type Prober struct { Install Install @@ -243,6 +274,11 @@ type Prober struct { PingTimeout time.Duration // StartupWindow bounds StateStarting; zero means DefaultStartupWindow. StartupWindow time.Duration + // Listening reports whether anything holds the connector port. Nil + // classifies from the ping error alone; the CLI sets it to ListeningOn, + // because a ping's error can name one refused address while another + // address has a listener behind it. + Listening func(ctx context.Context) bool } func (p *Prober) startupWindow() time.Duration { @@ -255,13 +291,14 @@ func (p *Prober) startupWindow() time.Duration { // Probe runs one presence check. It never fails: an undecidable signal is // reported in the Status rather than returned as an error. func (p *Prober) Probe(ctx context.Context) Status { - return p.probe(ctx, time.Time{}) + return p.probe(ctx, time.Time{}, 0) } // probe runs one check. firstSeen is when the caller first saw the lock held; // it stands in for the lock time when the platform could not report one, so a // long-held lock of unknown age still leaves "starting" eventually. -func (p *Prober) probe(ctx context.Context, firstSeen time.Time) Status { +// pingTimeout overrides the Prober's bound when positive. +func (p *Prober) probe(ctx context.Context, firstSeen time.Time, pingTimeout time.Duration) Status { st := Status{ ConnectorURL: p.ConnectorURL, Profiles: make([]ProfileStatus, 0, len(p.Install.Profiles)), @@ -290,7 +327,10 @@ func (p *Prober) probe(ctx context.Context, firstSeen time.Time) Status { pingErr = ErrConnectorUnavailable } } else { - timeout := p.PingTimeout + timeout := pingTimeout + if timeout <= 0 { + timeout = p.PingTimeout + } if timeout <= 0 { timeout = DefaultPingTimeout } @@ -319,11 +359,11 @@ func (p *Prober) probe(ctx context.Context, firstSeen time.Time) Status { switch { case since.IsZero() || now.Sub(since) < p.startupWindow(): st.State = StateStarting - case p.Ping == nil || connectorRefused(pingErr): + case p.Ping == nil || !p.listening(ctx, pingErr): // No connector zotio can address counts as none listening. st.State = StateConnectorOff default: - st.State = StateUnresponsive + st.State = StateBusy } default: st.State, st.Evidence = StateStopped, EvidenceNone @@ -341,6 +381,73 @@ func (p *Prober) startingUntil(st Status, firstSeen time.Time) time.Time { return since.Add(p.startupWindow()) } +// listening reports whether anything holds the connector port after a +// failed ping. +func (p *Prober) listening(ctx context.Context, pingErr error) bool { + if p.Listening != nil { + return p.Listening(ctx) + } + return !connectorRefused(pingErr) +} + +// ListeningOn reports whether anything holds the port of rawURL: it dials +// every address the host resolves to, a few times each, and answers false +// only when every dial is refused. +// +// A ping error is not enough. Measured 2026-09-24 against a hung Zotero 7 on +// macOS: Zotero listened on 127.0.0.1:23119 only; with its main thread +// blocked, the kernel completed the handshake on 127.0.0.1 and reset the +// connection, while [::1] refused. Go dials "localhost" in resolver order and +// reports the FIRST address's error when all fail, so the ping said +// "connection refused" for a port Zotero held. A completed connect, a reset +// after it, or a dial timeout (a full backlog drops SYNs) all mean a +// listener exists. The same hung listener also REFUSED some connects made +// right after a reset (2 of 3 back-to-back checks), so each address gets +// listenAttempts tries before it counts as refused. +func ListeningOn(ctx context.Context, rawURL string) bool { + u, err := url.Parse(rawURL) + if err != nil || u.Host == "" { + return false + } + port := u.Port() + if port == "" { + port = "80" + if u.Scheme == "https" { + port = "443" + } + } + addrs, err := net.DefaultResolver.LookupHost(ctx, u.Hostname()) + if err != nil || len(addrs) == 0 { + return false + } + d := net.Dialer{Timeout: time.Second} + for attempt := range listenAttempts { + if attempt > 0 { + select { + case <-ctx.Done(): + return true // undecided: do not claim nothing listens + case <-time.After(listenRetryDelay): + } + } + for _, addr := range addrs { + conn, err := d.DialContext(ctx, "tcp", net.JoinHostPort(addr, port)) + if err == nil { + _ = conn.Close() + return true + } + if !connectorRefused(err) { + return true + } + } + } + return false +} + +const ( + listenAttempts = 5 + listenRetryDelay = 100 * time.Millisecond +) + // connectorRefused reports whether a ping failed because nothing listens on // the port: the dial itself failed. A timeout, including a dial timeout, is // the opposite case: something holds the port and does not answer. diff --git a/internal/desktop/presence_test.go b/internal/desktop/presence_test.go index 861014d4..0323c542 100644 --- a/internal/desktop/presence_test.go +++ b/internal/desktop/presence_test.go @@ -128,13 +128,15 @@ func TestProbeStatesFollowTheTwoSignals(t *testing.T) { // The live case, 2026-09-24: Zotero up for well over the startup window, // its connector port accepting connections and never answering. - t.Run("unresponsive past the startup window", func(t *testing.T) { + // One silent check past the window is not evidence of a hang: a single + // probe reports busy, never unresponsive (only Wait can see a stall). + t.Run("busy past the startup window", func(t *testing.T) { dir := t.TempDir() startLockHolder(t, dir) for _, startup := range []time.Duration{time.Hour, time.Nanosecond} { p := &Prober{Install: Install{Profiles: []string{dir}}, Ping: noAnswer, StartupWindow: startup} st := p.Probe(t.Context()) - want := StateUnresponsive + want := StateBusy if startup == time.Hour { want = StateStarting // the same silence inside the window is a start } @@ -282,3 +284,113 @@ func TestConnectorRefusedClassifiesRealPingErrors(t *testing.T) { t.Fatalf("404 ping error = %v; want a failure that is not a refusal", err) } } + +// Zotero's connector runs on its main thread, which a large sync can hold for +// seconds. A connector that stalls for 10s and then answers must end the wait +// as ready, never as unresponsive, under the production stall evidence +// (60s span, 3 checks, 10s confirming pings). Only the re-check cadence is +// shortened, so the test takes about 10s rather than 20s. +func TestWaitTreatsATenSecondStallAsBusyNotUnresponsive(t *testing.T) { + if testing.Short() { + t.Skip("runs a real 10s connector stall") + } + dir := t.TempDir() + startLockHolder(t, dir) + stallUntil := time.Now().Add(10 * time.Second) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-time.After(time.Until(stallUntil)): + w.WriteHeader(http.StatusOK) + case <-r.Context().Done(): + } + })) + defer srv.Close() + conn := connector.New(srv.URL+"/connector", time.Minute) + prober := &Prober{ + Install: Install{Profiles: []string{dir}}, + Ping: conn.Ping, + StartupWindow: time.Nanosecond, // Zotero has been up for hours + } + ctx, cancel := context.WithTimeout(t.Context(), 45*time.Second) + defer cancel() + st, err := Wait(ctx, WaitOptions{Prober: prober, WatchDirs: []string{dir}, StallRecheck: 2 * time.Second}) + if err != nil || st.State != StateReady { + t.Fatalf("Wait across a 10s stall = %+v, %v; want ready", st, err) + } + if time.Now().Before(stallUntil) { + t.Fatal("Wait returned ready before the stall ended") + } +} + +// resetListener reproduces the hung Zotero measured 2026-09-24: a listener on +// 127.0.0.1 only, whose connections complete the handshake and are then +// reset. Dialled as "localhost", [::1] refuses first. +func resetListener(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp4", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = ln.Close() }) + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + if tc, ok := c.(*net.TCPConn); ok { + _ = tc.SetLinger(0) // close with RST + } + _ = c.Close() + } + }() + _, port, _ := net.SplitHostPort(ln.Addr().String()) + return "http://localhost:" + port + "/connector" +} + +func TestListeningOnSeesAListenerBehindARefusedAddress(t *testing.T) { + if !ListeningOn(t.Context(), resetListener(t)) { + t.Fatal("ListeningOn = false for a port held on 127.0.0.1 behind a refusing [::1]") + } + closed := realRefusalURL(t) + if ListeningOn(t.Context(), closed) { + t.Fatalf("ListeningOn(%s) = true for a closed port", closed) + } +} + +// The misclassification seen live: the ping reported "connection refused" +// (from [::1]) while Zotero held the port on 127.0.0.1. That is a hung +// Zotero, not a disabled connector, and the user must not be told to enable +// the connector setting. +func TestARefusedPingWithAListenerBehindItIsBusyNotConnectorOff(t *testing.T) { + dir := t.TempDir() + startLockHolder(t, dir) + base := resetListener(t) + misleading := &net.OpError{Op: "dial", Net: "tcp", Err: errors.New("connect: connection refused")} + p := &Prober{ + Install: Install{Profiles: []string{dir}}, + Ping: func(context.Context) error { return fmt.Errorf("connector ping: %w", misleading) }, + ConnectorURL: base, + StartupWindow: time.Nanosecond, + Listening: func(ctx context.Context) bool { return ListeningOn(ctx, base) }, + } + if st := p.Probe(t.Context()); st.State != StateBusy { + t.Fatalf("status = %+v, want busy: something holds the connector port", st) + } + closed := realRefusalURL(t) + p.Listening = func(ctx context.Context) bool { return ListeningOn(ctx, closed) } + if st := p.Probe(t.Context()); st.State != StateConnectorOff { + t.Fatalf("status = %+v, want connector_off when every address refuses", st) + } +} + +func realRefusalURL(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + _, port, _ := net.SplitHostPort(ln.Addr().String()) + _ = ln.Close() + return "http://localhost:" + port + "/connector" +} diff --git a/internal/desktop/wait.go b/internal/desktop/wait.go index 44cad09c..407927a8 100644 --- a/internal/desktop/wait.go +++ b/internal/desktop/wait.go @@ -37,7 +37,8 @@ const ( DefaultConfirmMax = 2 * time.Second ) -// WaitOptions configures Wait. The startup window is the Prober's. +// WaitOptions configures Wait. The startup window is the Prober's; zero +// durations and counts take the package defaults. type WaitOptions struct { Prober *Prober // WatchDirs are watched for changes; see Install.WatchDirs. @@ -47,6 +48,12 @@ type WaitOptions struct { ConfirmFirst time.Duration ConfirmMax time.Duration + // Sustained-stall evidence for StateUnresponsive; see DefaultStallSpan. + StallSpan time.Duration + StallChecks int + StallRecheck time.Duration + StallPingTimeout time.Duration + // OnWatching, when set, runs once the watches are installed and the // first probe found the connector down. Tests use it to know that a // later filesystem change will be seen. @@ -63,6 +70,18 @@ func (o WaitOptions) withDefaults() WaitOptions { if o.ConfirmMax < o.ConfirmFirst { o.ConfirmMax = max(DefaultConfirmMax, o.ConfirmFirst) } + if o.StallSpan <= 0 { + o.StallSpan = DefaultStallSpan + } + if o.StallChecks <= 0 { + o.StallChecks = DefaultStallChecks + } + if o.StallRecheck <= 0 { + o.StallRecheck = DefaultStallRecheck + } + if o.StallPingTimeout <= 0 { + o.StallPingTimeout = DefaultStallPingTimeout + } return o } @@ -75,9 +94,17 @@ func (o WaitOptions) withDefaults() WaitOptions { // a change settles. While Zotero is starting (lock held, connector silent, // lock younger than the startup window) the connector is re-checked on a // capped backoff, because it listens seconds after the lock and its start -// writes no file. When the startup window passes with the connector still -// silent, or when Zotero is already past it, Wait returns ErrStuck with the -// unresponsive or connector_off Status. +// writes no file. +// +// Past the startup window a connector that cannot take a request is +// re-checked every StallRecheck with the longer StallPingTimeout. The wait +// ends with ErrStuck only once StallChecks consecutive checks have failed +// across at least StallSpan: as StateUnresponsive if any of them found a +// listener on the connector port, as StateConnectorOff if none did. Any +// answer in between ends it as ready, so a sync that stalls Zotero briefly +// is never reported as a hang. While Zotero is starting or busy, the +// re-check timer alone drives probes; filesystem events (a sync writes its +// WAL constantly) do not add more. // // On cancellation Wait returns the last Status and context.Cause(ctx), so a // caller that set a deadline with context.WithTimeoutCause can tell a @@ -95,16 +122,45 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { defer watcher.Close() } - // firstSeen stands in for the lock time when the platform reports none. - var firstSeen time.Time - probe := func() Status { - st := o.Prober.probe(ctx, firstSeen) + var ( + // firstSeen stands in for the lock time when the platform reports none. + firstSeen time.Time + stallSince time.Time + stallFails int + // stallHeld: some check in the stall found a listener on the + // connector port, so the stall is a hang rather than a connector + // that is off. + stallHeld bool + ) + probe := func(pingTimeout time.Duration) Status { + st := o.Prober.probe(ctx, firstSeen, pingTimeout) switch { case !st.LockHeld(): firstSeen = time.Time{} case firstSeen.IsZero(): firstSeen = st.CheckedAt } + if st.State != StateBusy && st.State != StateConnectorOff { + stallSince, stallFails, stallHeld = time.Time{}, 0, false + return st + } + if stallSince.IsZero() { + stallSince = st.CheckedAt + } + stallFails++ + stallHeld = stallHeld || st.State == StateBusy + since := stallSince + st.StalledSince = &since + if stallFails >= o.StallChecks && st.CheckedAt.Sub(stallSince) >= o.StallSpan { + if stallHeld { + st.State = StateUnresponsive + } else { + st.State = StateConnectorOff + } + return st + } + // Not proven yet either way: keep waiting. + st.State = StateBusy return st } // settled reports whether st ends the wait, and how. @@ -119,11 +175,11 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { } } - st := probe() + st := probe(0) if done, err := settled(st); done { return st, err } - if watcher == nil { + if watcher == nil && st.State != StateBusy && st.State != StateStarting { if watchErr == nil { return st, ErrNoProfile } @@ -135,8 +191,9 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { var ( settle *time.Timer - confirm *time.Timer + recheck *time.Timer backoff time.Duration + phase State ) stopTimer := func(t **time.Timer) { if *t != nil { @@ -145,77 +202,96 @@ func Wait(ctx context.Context, opts WaitOptions) (Status, error) { } } defer stopTimer(&settle) - defer stopTimer(&confirm) + defer stopTimer(&recheck) timerC := func(t *time.Timer) <-chan time.Time { if t == nil { return nil } return t.C } - // nextConfirm schedules the next re-check: the backoff step, but never - // past the end of the startup window, so leaving "starting" is noticed - // when it happens rather than up to one step later. - nextConfirm := func() { - delay := backoff - if until := time.Until(o.Prober.startingUntil(st, firstSeen)); until < delay { - delay = max(until, 0) + 10*time.Millisecond + // reschedule arms the re-check timer for the state just observed: + // backoff while starting (never past the end of the startup window, so + // leaving it is noticed when it happens), the stall cadence while busy + // (never past the end of the stall span), nothing otherwise. + reschedule := func() { + stopTimer(&recheck) + var delay time.Duration + switch st.State { + case StateStarting: + if phase == StateStarting { + backoff = min(backoff*2, o.ConfirmMax) + } else { + backoff = o.ConfirmFirst + } + delay = backoff + if until := time.Until(o.Prober.startingUntil(st, firstSeen)); until < delay { + delay = max(until, 0) + 10*time.Millisecond + } + case StateBusy: + delay = o.StallRecheck + if until := time.Until(stallSince.Add(o.StallSpan)); until > 0 && until < delay { + delay = until + 10*time.Millisecond + } } - confirm = time.NewTimer(delay) - } - // track keeps the re-checks running exactly while Zotero is starting. - track := func() { - if st.State != StateStarting { - stopTimer(&confirm) - return + phase = st.State + if delay > 0 { + recheck = time.NewTimer(delay) } - if confirm == nil { - backoff = o.ConfirmFirst - nextConfirm() + } + reschedule() + + // A nil watcher (nothing watchable, but Zotero already up) leaves the + // event cases blocked forever, which is what they should be. + var events <-chan fsnotify.Event + var watchErrs <-chan error + if watcher != nil { + events, watchErrs = watcher.Events, watcher.Errors + } + onEvent := func() { + // While starting or busy the re-check timer drives probes. + if recheck == nil && settle == nil { + settle = time.NewTimer(o.Settle) } } - track() for { select { case <-ctx.Done(): return st, context.Cause(ctx) - case _, ok := <-watcher.Events: + case _, ok := <-events: if !ok { return st, errors.New("filesystem watcher closed") } - if settle == nil { - settle = time.NewTimer(o.Settle) - } + onEvent() - case _, ok := <-watcher.Errors: + case _, ok := <-watchErrs: if !ok { return st, errors.New("filesystem watcher closed") } // An overflow or read error means events may have been lost: // probe as though one arrived rather than trusting the silence. - if settle == nil { - settle = time.NewTimer(o.Settle) - } + onEvent() case <-timerC(settle): settle = nil - st = probe() + st = probe(0) if done, err := settled(st); done { return st, err } - track() + reschedule() - case <-timerC(confirm): - confirm = nil - st = probe() + case <-timerC(recheck): + recheck = nil + pingTimeout := time.Duration(0) + if phase == StateBusy { + pingTimeout = o.StallPingTimeout + } + st = probe(pingTimeout) if done, err := settled(st); done { return st, err } - if st.State == StateStarting { - backoff = min(backoff*2, o.ConfirmMax) - nextConfirm() - } + reschedule() } } } diff --git a/internal/desktop/wait_test.go b/internal/desktop/wait_test.go index 9ba03de8..6b88c2a3 100644 --- a/internal/desktop/wait_test.go +++ b/internal/desktop/wait_test.go @@ -194,16 +194,24 @@ func TestWaitConfirmsTheConnectorAfterTheLockWithoutAFurtherEvent(t *testing.T) h.release(t) } +// stallTuning shrinks the sustained-stall evidence to test scale. +func stallTuning(o *WaitOptions) { + o.StallSpan, o.StallChecks = 300*time.Millisecond, 3 + o.StallRecheck, o.StallPingTimeout = 50*time.Millisecond, 20*time.Millisecond +} + // The live case: Zotero starts, takes its lock, and hangs before its -// connector answers. Once the startup window passes, Wait must return the -// unresponsive status rather than wait silently; no filesystem change would -// ever announce a recovery. -func TestWaitReturnsStuckWhenTheStartupWindowPassesWithoutAnAnswer(t *testing.T) { +// connector answers. After the startup window it is busy; once the silence +// has lasted the stall span over enough checks, Wait returns the +// unresponsive status rather than wait silently, since no filesystem change +// would ever announce a recovery. +func TestWaitReturnsUnresponsiveOnlyAfterASustainedStall(t *testing.T) { in := newInstall(t) conn := &fakeConnector{fail: errNoAnswer} done := startWait(t, t.Context(), in, conn, func(o *WaitOptions) { o.ConfirmFirst, o.ConfirmMax = 10*time.Millisecond, 40*time.Millisecond - o.Prober.StartupWindow = 400 * time.Millisecond + o.Prober.StartupWindow = 300 * time.Millisecond + stallTuning(o) }) locked := time.Now() @@ -212,43 +220,108 @@ func TestWaitReturnsStuckWhenTheStartupWindowPassesWithoutAnAnswer(t *testing.T) if !errors.Is(r.err, ErrStuck) || r.st.State != StateUnresponsive || !r.st.Running { t.Fatalf("Wait = %+v, %v; want ErrStuck with the unresponsive status", r.st, r.err) } - // Not before the window: a slow start is still a start. - if elapsed := time.Since(locked); elapsed < 300*time.Millisecond { - t.Fatalf("Wait gave up %v after the lock, inside the 400ms startup window", elapsed) + // Not before the startup window plus the stall span. + if elapsed := time.Since(locked); elapsed < 550*time.Millisecond { + t.Fatalf("Wait gave up %v after the lock; the window (300ms) plus the stall span (300ms) had not passed", elapsed) + } + if r.st.StalledSince == nil || r.st.CheckedAt.Sub(*r.st.StalledSince) < 300*time.Millisecond { + t.Fatalf("stalled_since = %v at %v, want at least the stall span before", r.st.StalledSince, r.st.CheckedAt) + } +} + +// A stall shorter than the span, followed by an answer, is a busy Zotero, +// not a hung one: the wait ends as ready. +func TestWaitRidesOutAStallThatEndsBeforeTheSpan(t *testing.T) { + in := newInstall(t) + startLockHolder(t, in.profile) + conn := &fakeConnector{fail: errNoAnswer} + done := startWait(t, t.Context(), in, conn, func(o *WaitOptions) { + o.Prober.StartupWindow = time.Nanosecond + stallTuning(o) + o.StallSpan = 2 * time.Second + }) + time.Sleep(300 * time.Millisecond) // several failed stall checks + if conn.count() < 3 { + t.Fatalf("pings during the stall = %d, want re-checks", conn.count()) + } + conn.setUp() + if r := awaitResult(t, done); r.err != nil || r.st.State != StateReady { + t.Fatalf("Wait = %+v, %v; want ready once the stall ends", r.st, r.err) + } +} + +// A connector that is off refuses on every check. That too needs the +// sustained span, because a hung Zotero's listener also refuses some +// connects; once proven, the state names the connector, not a hang. +func TestWaitReturnsConnectorOffAfterASustainedRefusal(t *testing.T) { + in := newInstall(t) + startLockHolder(t, in.profile) + conn := &fakeConnector{fail: realRefusal(t)} + done := startWait(t, t.Context(), in, conn, func(o *WaitOptions) { + o.Prober.StartupWindow = time.Nanosecond + stallTuning(o) + }) + r := awaitResult(t, done) + if !errors.Is(r.err, ErrStuck) || r.st.State != StateConnectorOff { + t.Fatalf("Wait = %+v, %v; want ErrStuck in state connector_off", r.st, r.err) } if conn.count() < 3 { - t.Fatalf("pings = %d, want re-checks during the startup window", conn.count()) + t.Fatalf("pings = %d, want the sustained-stall checks", conn.count()) } } -// Zotero already hung (or running with its connector disabled) when the -// wait begins: the answer is immediate, and names which of the two it is. -func TestWaitReturnsStuckAtOnceWhenZoteroIsAlreadyPastItsStart(t *testing.T) { - cases := []struct { - name string - fail func(t *testing.T) error - want State - }{ - {"hung", func(*testing.T) error { return errNoAnswer }, StateUnresponsive}, - {"connector disabled", realRefusal, StateConnectorOff}, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - in := newInstall(t) - startLockHolder(t, in.profile) - conn := &fakeConnector{fail: tc.fail(t)} - prober := &Prober{Install: Install{Profiles: []string{in.profile}, DataDir: in.data}, Ping: conn.ping, StartupWindow: time.Nanosecond} - ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) - defer cancel() - st, err := Wait(ctx, WaitOptions{Prober: prober, WatchDirs: []string{in.profile, in.data}}) - if !errors.Is(err, ErrStuck) || st.State != tc.want { - t.Fatalf("Wait = %+v, %v; want ErrStuck in state %s", st, err, tc.want) - } - if conn.count() != 1 { - t.Fatalf("pings = %d, want 1: the first probe already decides", conn.count()) - } - }) +// A stall in which any check found a listener is a hang, even if other +// checks were refused (the intermittent refusals of a hung listener). +func TestWaitCallsAMixedStallUnresponsive(t *testing.T) { + in := newInstall(t) + startLockHolder(t, in.profile) + refused := realRefusal(t) + var mu sync.Mutex + n := 0 + ping := func(context.Context) error { + mu.Lock() + defer mu.Unlock() + n++ + if n%2 == 0 { + return errNoAnswer + } + return refused } + prober := &Prober{Install: Install{Profiles: []string{in.profile}, DataDir: in.data}, Ping: ping, StartupWindow: time.Nanosecond} + opts := WaitOptions{Prober: prober, WatchDirs: []string{in.profile, in.data}} + stallTuning(&opts) + ctx, cancel := context.WithTimeout(t.Context(), 10*time.Second) + defer cancel() + st, err := Wait(ctx, opts) + if !errors.Is(err, ErrStuck) || st.State != StateUnresponsive { + t.Fatalf("Wait = %+v, %v; want ErrStuck in state unresponsive", st, err) + } +} + +// Busy re-checks are timer-driven; a sync writing its WAL constantly must +// not turn every filesystem event into another ping of a busy Zotero. +func TestWaitIgnoresEventsWhileBusy(t *testing.T) { + in := newInstall(t) + startLockHolder(t, in.profile) + conn := &fakeConnector{fail: errNoAnswer} + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + done := startWait(t, ctx, in, conn, func(o *WaitOptions) { + o.Prober.StartupWindow = time.Nanosecond + o.StallSpan, o.StallRecheck = time.Hour, time.Hour + }) + for i := range 20 { + if err := os.WriteFile(filepath.Join(in.data, "zotero.sqlite-wal"), []byte{byte(i)}, 0o600); err != nil { + t.Fatal(err) + } + time.Sleep(20 * time.Millisecond) + } + assertStillWaiting(t, done, 200*time.Millisecond) + if got := conn.count(); got != 1 { + t.Fatalf("pings while busy with WAL events = %d, want only the first", got) + } + cancel() + awaitResult(t, done) } func TestWaitTimeoutReturnsItsCause(t *testing.T) { diff --git a/internal/mcp/testdata/surface_mirror.golden.json b/internal/mcp/testdata/surface_mirror.golden.json index 635b1e40..26bc3bf6 100644 --- a/internal/mcp/testdata/surface_mirror.golden.json +++ b/internal/mcp/testdata/surface_mirror.golden.json @@ -709,7 +709,7 @@ }, { "name": "desktop_status", - "description": "Report whether Zotero desktop is running and whether its connector accepts\nrequests. Cheap and local: it reads the profile lock of every discovered\nZotero profile and sends one ping to the local connector. It exits 0 whatever\nit finds; read the fields, not the exit code.\n\nTwo signals, reported separately:\n\n running Zotero's process is up: another process holds the\n profile lock (.parentlock via fcntl on macOS and Linux,\n parent.lock opened exclusively on Windows), or the\n connector answered.\n connector_reachable GET \u003cconnector\u003e/ping answered 200 within 3s during this\n check. Imports and every other connector write need\n this.\n\nstate:\n ready the connector answers.\n starting the lock is held, the connector does not answer yet, and the\n lock is younger than the 2-minute startup window. Zotero\n takes the lock about 3s after launch and its connector\n listens a few seconds later.\n unresponsive the lock is older than the startup window and the connector\n port accepts the connection but does not answer (or answers\n with an error): Zotero is open but not responding.\n connector_off the lock is older than the startup window and nothing\n listens on the connector port: the connector is disabled\n (Settings -\u003e Advanced -\u003e \"Allow other applications to\n communicate with Zotero\") or on another port.\n stopped neither signal holds.\n\nThe lock age comes from the lock file's modification time, which Zotero\nresets when it takes the lock (profiles[].lock_since). evidence names the\nstrongest signal: connector, profile_lock, none.\n\nProfiles are discovered from profiles.ini in the platform's Zotero directory\n(ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js.", + "description": "Report whether Zotero desktop is running and whether its connector accepts\nrequests. Cheap and local: it reads the profile lock of every discovered\nZotero profile and sends one ping to the local connector. It exits 0 whatever\nit finds; read the fields, not the exit code.\n\nTwo signals, reported separately:\n\n running Zotero's process is up: another process holds the\n profile lock (.parentlock via fcntl on macOS and Linux,\n parent.lock opened exclusively on Windows), or the\n connector answered.\n connector_reachable GET \u003cconnector\u003e/ping answered 200 within 3s during this\n check. Imports and every other connector write need\n this.\n\nstate:\n ready the connector answers.\n starting the lock is held, the connector does not answer yet, and the\n lock is younger than the 2-minute startup window. Zotero\n takes the lock about 3s after launch and its connector\n listens a few seconds later.\n busy the lock is older than the startup window and the connector\n port accepted the connection but did not answer in this\n check (or answered with an error). One silent check is not\n a hang: a large sync can hold Zotero's main thread for\n seconds.\n unresponsive desktop wait only: the connector stayed silent for at least\n 60s across 3 or more checks (each allowed 10s): Zotero is\n open but not responding.\n connector_off the lock is older than the startup window and nothing\n listens on the connector port (every address refused\n repeated connects): the connector is disabled (Settings -\u003e\n Advanced -\u003e \"Allow other applications to communicate with\n Zotero\") or on another port.\n stopped neither signal holds.\n\nThe lock age comes from the lock file's modification time, which Zotero\nresets when it takes the lock (profiles[].lock_since). evidence names the\nstrongest signal: connector, profile_lock, none.\n\nProfiles are discovered from profiles.ini in the platform's Zotero directory\n(ZOTERO_PROFILE_DIR pins one); data_dir comes from the profile's prefs.js.", "inputSchema": { "properties": {}, "required": [], From 057aa7f4e800f240034ef03a5081c7ddca132099 Mon Sep 17 00:00:00 2001 From: enieuwy <121954036+enieuwy@users.noreply.github.com> Date: Thu, 24 Sep 2026 21:06:33 +0800 Subject: [PATCH 4/5] docs(skill): name the commands behind exit 11 The exit-code table now names `desktop wait` (exits 14 and 15), so the SKILL drift test scopes the table's bare flags to that command and rejects --fail-on and --fail-on-unknown. Name the commands that own them: library health and items bibcheck. --- SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SKILL.md b/SKILL.md index 440bcd74..3ff45afd 100644 --- a/SKILL.md +++ b/SKILL.md @@ -285,7 +285,7 @@ Explicit flags always win over profile values; profile values win over defaults. | 7 | Rate limited (wait and retry) | | 9 | Precondition unmet, or a writer lock is held | | 10 | Config error | -| 11 | Quality gate failed (`--fail-on`, `--fail-on-unknown`) | +| 11 | Quality gate failed (`library health --fail-on`, `items bibcheck --fail-on`/`--fail-on-unknown`) | | 12 | Stale data | | 13 | Incomplete — part succeeded, part was rejected; reconcile before retrying | | 14 | Timed out — `desktop wait --timeout` ended before Zotero's connector answered; wait again | From 1f0928b85df95f7489bd3e99046db79ff8fa315d Mon Sep 17 00:00:00 2001 From: enieuwy <121954036+enieuwy@users.noreply.github.com> Date: Thu, 24 Sep 2026 21:22:49 +0800 Subject: [PATCH 5/5] fix(desktop): let stall re-checks use their 10s ping The prober's connector client carried a 3s http.Client timeout, which caps a request regardless of its context deadline. desktop wait gives each stall re-check a 10s ping, so every re-check was cut at 3s and a Zotero answering in 4s during a sync counted as silent toward exit 15. The client now allows the longest probe bound and each probe's context deadline governs. Help text states both bounds. Found by CodeRabbit on #49. --- docs/reference/commands.md | 2 +- internal/cli/desktop.go | 7 +++++-- internal/cli/desktop_test.go | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 3 deletions(-) diff --git a/docs/reference/commands.md b/docs/reference/commands.md index c33fe0db..c9451995 100644 --- a/docs/reference/commands.md +++ b/docs/reference/commands.md @@ -854,7 +854,7 @@ Exit codes and the JSON outcome field: is printed on stdout. --timeout replaces the global request timeout for this command; a connector -ping is always bounded to 3s. +ping is bounded to 3s, or 10s for the stall re-checks past the startup window. ``` zotio desktop wait [flags] diff --git a/internal/cli/desktop.go b/internal/cli/desktop.go index 39bf4260..d3ad2e03 100644 --- a/internal/cli/desktop.go +++ b/internal/cli/desktop.go @@ -159,7 +159,7 @@ Exit codes and the JSON outcome field: is printed on stdout. --timeout replaces the global request timeout for this command; a connector -ping is always bounded to 3s.`, +ping is bounded to 3s, or 10s for the stall re-checks past the startup window.`, Example: ` zotio desktop wait zotio desktop wait --agent --timeout 6h # Supervised: exit when the supervisor's pipe closes @@ -268,7 +268,10 @@ func newDesktopProber(flags *rootFlags) (*desktop.Prober, error) { prober.ConnectorErr = fmt.Errorf("the desktop connector is only available with a local Zotero base URL") return prober, nil } - conn := connector.New(base, desktop.DefaultPingTimeout) + // The client timeout is only a backstop: each probe sets its own context + // deadline (3s, or 10s for a stall re-check), so the client must allow + // the longest of them or it silently caps every stall ping at 3s. + conn := connector.New(base, max(desktop.DefaultPingTimeout, desktop.DefaultStallPingTimeout)) prober.ConnectorURL = base prober.Ping = func(ctx context.Context) error { return connectorPing(ctx, conn) } prober.Listening = func(ctx context.Context) bool { return desktop.ListeningOn(ctx, base) } diff --git a/internal/cli/desktop_test.go b/internal/cli/desktop_test.go index 0a9a4f62..6312d241 100644 --- a/internal/cli/desktop_test.go +++ b/internal/cli/desktop_test.go @@ -7,6 +7,8 @@ import ( "context" "encoding/json" "errors" + "net/http" + "net/http/httptest" "os" "path/filepath" "slices" @@ -270,3 +272,34 @@ func TestDesktopWaitStuckExits15WithTheState(t *testing.T) { }) } } + +// A stall re-check allows the connector 10s to answer, because a sync can +// hold Zotero's main thread past the 3s first-check bound. The prober's +// connector must honour that longer deadline; a client capped at 3s would +// count a Zotero that answers in 4s as silent and drive it toward exit 15. +func TestDesktopProberHonoursTheStallPingDeadline(t *testing.T) { + flags, _ := desktopTestEnv(t, true, true) + delay := desktop.DefaultPingTimeout + 500*time.Millisecond + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + select { + case <-time.After(delay): + case <-r.Context().Done(): + return + } + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + connectorPing = func(ctx context.Context, c *connector.Client) error { + c.BaseURL = srv.URL + "/connector" + return c.Ping(ctx) + } + prober, err := newDesktopProber(flags) + if err != nil { + t.Fatalf("newDesktopProber: %v", err) + } + ctx, cancel := context.WithTimeout(t.Context(), desktop.DefaultStallPingTimeout) + defer cancel() + if err := prober.Ping(ctx); err != nil { + t.Fatalf("ping answering after %s under a %s deadline = %v, want nil", delay, desktop.DefaultStallPingTimeout, err) + } +}