From b81e3a217136602e9fb5a1219467b78f26deda47 Mon Sep 17 00:00:00 2001 From: napalm255 Date: Sat, 26 Sep 2026 20:54:04 -0400 Subject: [PATCH 01/19] fix(io): report a failed local save as a file error, not a daemon one A received file that could not be written here (a full disk, a read-only or missing download directory, a name too long for the file system) was reported as the Tailscale daemon being unreachable, because io.js mapped every Gio code as if it came from the socket. Add REASON.LOCAL_FILE ("Could not save the file.") and move the Gio code to REASON mapping into modules/errors.js as reasonForIoError, which is told what the failed operation touched. saveFile now blames the file for every create_async failure, for a splice_async failure with NO_SPACE, READ_ONLY, PERMISSION_DENIED or NOT_FOUND, and for running out of candidate names. --- modules/errors.js | 65 +++++++++++++++++-- modules/io.js | 53 +++++++++------- modules/menu-items.js | 2 + tests/errors.test.js | 113 +++++++++++++++++++++++++++++++-- tests/model.test.js | 19 ++++++ tests/taildrop-section.test.js | 25 ++++++++ 6 files changed, 244 insertions(+), 33 deletions(-) diff --git a/modules/errors.js b/modules/errors.js index 1ee569a..1180cbd 100644 --- a/modules/errors.js +++ b/modules/errors.js @@ -1,11 +1,12 @@ // What went wrong talking to tailscaled, as a value rather than a string. // -// This file imports nothing. modules/io.js is the only place that can tell a -// Gio.IOErrorEnum apart from an HTTP status, because it is the only place with -// Gio in scope, so it does that translation and throws one of these. Everything -// downstream — the reducer, the menu, the tests — reasons about the symbol. +// This file imports nothing. modules/io.js is the only place with Gio in +// scope, so it reads the code off a GError and hands it here, with +// Gio.IOErrorEnum passed in rather than imported; the reason is decided here +// and io.js throws it. Everything downstream — the reducer, the menu, the +// tests — reasons about the symbol. // -// The division is deliberate: io.js knows Gio, this file knows what to say. +// The division is deliberate: io.js knows Gio, this file knows what it means. /** * Why a request failed. @@ -24,6 +25,8 @@ export const REASON = Object.freeze({ HTTP: 'http', /** The daemon answered with something that is not what it claimed to be. */ PROTOCOL: 'protocol', + /** A file here could not be written: a full disk, an unwritable directory. */ + LOCAL_FILE: 'local-file', /** Anything else. */ UNKNOWN: 'unknown', }); @@ -60,6 +63,56 @@ export function reasonOf(error) { return error?.name === 'TransportError' ? error.reason : REASON.UNKNOWN; } +/** + * The reason to report for a failed Gio operation. + * + * A socket and a file fail with the same codes and mean different things by + * them. NOT_FOUND is no daemon on the one and no download directory on the + * other; PERMISSION_DENIED is a missing operator on the one and a directory + * this user cannot write on the other. Only the caller knows which it was + * talking to, so it says. + * + * Gio.IOErrorEnum is passed in, as prefs.js passes its Gtk values to + * modules/shortcuts.js, so this stays importable on plain Node. + * + * @param {number|null} code The GError's code, or null when the error is not + * one of Gio.IOErrorEnum's. + * @param {object} IOErrorEnum Gio.IOErrorEnum. + * @param {object} [where] What the failed operation touched. + * @param {boolean} [where.local] A file on this machine. + * @param {boolean} [where.remote] The daemon: its socket, or a stream from it. + * @returns {string} One of {@link REASON}. + */ +export function reasonForIoError( + code, + IOErrorEnum, + { local = false, remote = true } = {}, +) { + // Creating a file touches nothing else, so whatever failed, it was the file. + if (local && !remote) return REASON.LOCAL_FILE; + + // A copy from the daemon's stream into a file. The connection is already + // open by then, so these can only be the file's; the rest are the stream's. + const fileSystem = [ + IOErrorEnum.NO_SPACE, + IOErrorEnum.READ_ONLY, + IOErrorEnum.PERMISSION_DENIED, + IOErrorEnum.NOT_FOUND, + ]; + if (local && fileSystem.includes(code)) return REASON.LOCAL_FILE; + + switch (code) { + case IOErrorEnum.NOT_FOUND: + return REASON.SOCKET_MISSING; + case IOErrorEnum.CONNECTION_REFUSED: + return REASON.CONNECTION_REFUSED; + case IOErrorEnum.PERMISSION_DENIED: + return REASON.PERMISSION_DENIED; + default: + return REASON.UNKNOWN; + } +} + /** The one command that fixes a permission failure. */ const OPERATOR_COMMAND = 'sudo tailscale set --operator=$USER'; @@ -88,6 +141,8 @@ export function messageFor(reason) { return 'The Tailscale daemon refused the request.'; case REASON.PROTOCOL: return 'The Tailscale daemon sent an unexpected response.'; + case REASON.LOCAL_FILE: + return 'Could not save the file.'; default: return 'Could not reach the Tailscale daemon.'; } diff --git a/modules/io.js b/modules/io.js index a5242bb..c43ce95 100644 --- a/modules/io.js +++ b/modules/io.js @@ -16,7 +16,7 @@ import GLib from 'gi://GLib'; import Soup from 'gi://Soup?version=3.0'; import { CanceledError } from './cancel.js'; -import { REASON, TransportError } from './errors.js'; +import { REASON, TransportError, reasonForIoError } from './errors.js'; import { candidateNames, isSafeFileName } from './inbox.js'; import { HOST, SOCKET_PATHS, pickSocket } from './localapi.js'; @@ -49,25 +49,26 @@ const REQUEST = 'org.freedesktop.portal.Request'; * is also where a canceled operation stops looking like a failure. Getting * that wrong would make every disable() log an error. * + * Which reason a Gio code stands for is modules/errors.js's reasonForIoError; + * this only reads the code off the GError. + * * @param {unknown} error Caught value. + * @param {{local?: boolean, remote?: boolean}} [where] What the failed + * operation touched, as reasonForIoError takes it. The daemon by default. * @returns {Error} A CanceledError or a TransportError. */ -function translate(error) { +function translate(error, where = {}) { if (error?.name === 'CanceledError' || error?.name === 'TransportError') return error; - if (error instanceof Gio.IOErrorEnum || typeof error?.matches === 'function') { - if (error.matches?.(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)) - return new CanceledError(); - if (error.matches?.(Gio.IOErrorEnum, Gio.IOErrorEnum.NOT_FOUND)) - return transportError(REASON.SOCKET_MISSING, error); - if (error.matches?.(Gio.IOErrorEnum, Gio.IOErrorEnum.CONNECTION_REFUSED)) - return transportError(REASON.CONNECTION_REFUSED, error); - if (error.matches?.(Gio.IOErrorEnum, Gio.IOErrorEnum.PERMISSION_DENIED)) - return transportError(REASON.PERMISSION_DENIED, error); - } + // GJS gives every Error a matches(), which is false for anything but a + // GError of that domain — so a plain exception gets no code at all. + if (error?.matches?.(Gio.IOErrorEnum, Gio.IOErrorEnum.CANCELLED)) + return new CanceledError(); + + const code = error?.matches?.(Gio.IOErrorEnum, error.code) ? error.code : null; - return transportError(REASON.UNKNOWN, error); + return transportError(reasonForIoError(code, Gio.IOErrorEnum, where), error); } /** @@ -614,22 +615,30 @@ export function createIo({ token }) { ); break; } catch (error) { + // Only the file was involved, so any other failure + // is the file's, never the daemon's. if (!error?.matches?.(Gio.IOErrorEnum, Gio.IOErrorEnum.EXISTS)) - throw error; + throw translate(error, { local: true, remote: false }); file = null; } } if (!output) - throw new TransportError(REASON.UNKNOWN, 'no free file name'); + throw new TransportError(REASON.LOCAL_FILE, 'no free file name'); - await output.splice_async( - input, - Gio.OutputStreamSpliceFlags.CLOSE_SOURCE | - Gio.OutputStreamSpliceFlags.CLOSE_TARGET, - GLib.PRIORITY_DEFAULT, - cancellable, - ); + // From the daemon's stream into the file, so a failure may be + // either's; reasonForIoError tells them apart by the code. + try { + await output.splice_async( + input, + Gio.OutputStreamSpliceFlags.CLOSE_SOURCE | + Gio.OutputStreamSpliceFlags.CLOSE_TARGET, + GLib.PRIORITY_DEFAULT, + cancellable, + ); + } catch (error) { + throw translate(error, { local: true }); + } return file.get_path(); } catch (error) { diff --git a/modules/menu-items.js b/modules/menu-items.js index 530a6ad..9cdd006 100644 --- a/modules/menu-items.js +++ b/modules/menu-items.js @@ -134,6 +134,8 @@ export function problemMessage(reason, _) { return _('The Tailscale daemon refused the request.'); case REASON.PROTOCOL: return _('The Tailscale daemon sent an unexpected response.'); + case REASON.LOCAL_FILE: + return _('Could not save the file.'); default: return _('Could not reach the Tailscale daemon.'); } diff --git a/tests/errors.test.js b/tests/errors.test.js index b077abb..8789520 100644 --- a/tests/errors.test.js +++ b/tests/errors.test.js @@ -3,11 +3,26 @@ import { describe, expect, it } from 'vitest'; import { REASON, TransportError, + commandFor, isActionable, messageFor, + reasonForIoError, reasonOf, } from '../modules/errors.js'; +// Stand-ins for the Gio.IOErrorEnum codes modules/io.js passes in, with the +// values GIO gives them. +const IO = Object.freeze({ + FAILED: 0, + NOT_FOUND: 1, + FILENAME_TOO_LONG: 9, + NO_SPACE: 12, + PERMISSION_DENIED: 14, + READ_ONLY: 21, + CONNECTION_REFUSED: 39, + CONNECTION_CLOSED: 44, +}); + describe('TransportError', () => { it('carries its reason and status', () => { const error = new TransportError(REASON.HTTP, 'HTTP 500', { status: 500 }); @@ -82,10 +97,96 @@ describe('isActionable', () => { // Telling someone the daemon is unreachable is noise; it belongs in a // subtitle, not a row of its own. - it.each([[REASON.CONNECTION_REFUSED], [REASON.HTTP], [REASON.UNKNOWN]])( - '%s is not', - reason => { - expect(isActionable(reason)).toBe(false); - }, - ); + it.each([ + [REASON.CONNECTION_REFUSED], + [REASON.HTTP], + [REASON.UNKNOWN], + [REASON.LOCAL_FILE], + ])('%s is not', reason => { + expect(isActionable(reason)).toBe(false); + }); +}); + +describe('commandFor', () => { + // No command fixes a full disk or a download directory that is not + // writable, and the operator command would be the wrong advice. + it('names no command for a local file failure', () => { + expect(commandFor(REASON.LOCAL_FILE)).toBe(''); + }); +}); + +describe('reasonForIoError', () => { + describe('talking to the daemon', () => { + it.each([ + { name: 'NOT_FOUND', code: IO.NOT_FOUND, reason: REASON.SOCKET_MISSING }, + { + name: 'CONNECTION_REFUSED', + code: IO.CONNECTION_REFUSED, + reason: REASON.CONNECTION_REFUSED, + }, + { + name: 'PERMISSION_DENIED', + code: IO.PERMISSION_DENIED, + reason: REASON.PERMISSION_DENIED, + }, + { + name: 'CONNECTION_CLOSED', + code: IO.CONNECTION_CLOSED, + reason: REASON.UNKNOWN, + }, + { name: 'FAILED', code: IO.FAILED, reason: REASON.UNKNOWN }, + ])('reports $name as $reason', ({ code, reason }) => { + expect(reasonForIoError(code, IO)).toBe(reason); + }); + + it('reports something that is not an I/O error as unknown', () => { + expect(reasonForIoError(null, IO)).toBe(REASON.UNKNOWN); + }); + }); + + // Only the file was involved, so whatever went wrong went wrong with the + // file. A name too long for the file system is not the daemon missing. + describe('creating a local file', () => { + it.each([ + { name: 'NOT_FOUND', code: IO.NOT_FOUND }, + { name: 'PERMISSION_DENIED', code: IO.PERMISSION_DENIED }, + { name: 'NO_SPACE', code: IO.NO_SPACE }, + { name: 'READ_ONLY', code: IO.READ_ONLY }, + { name: 'FILENAME_TOO_LONG', code: IO.FILENAME_TOO_LONG }, + { name: 'FAILED', code: IO.FAILED }, + ])('reports $name as the file', ({ code }) => { + expect(reasonForIoError(code, IO, { local: true, remote: false })).toBe( + REASON.LOCAL_FILE, + ); + }); + }); + + // A copy from the daemon's stream into the file: the codes only a file + // system reports are the file's, and the rest are the stream's. + describe('copying from the daemon into a local file', () => { + it.each([ + { name: 'NO_SPACE', code: IO.NO_SPACE }, + { name: 'READ_ONLY', code: IO.READ_ONLY }, + { name: 'PERMISSION_DENIED', code: IO.PERMISSION_DENIED }, + { name: 'NOT_FOUND', code: IO.NOT_FOUND }, + ])('reports $name as the file, not the daemon', ({ code }) => { + expect(reasonForIoError(code, IO, { local: true })).toBe(REASON.LOCAL_FILE); + }); + + it.each([ + { + name: 'CONNECTION_CLOSED', + code: IO.CONNECTION_CLOSED, + reason: REASON.UNKNOWN, + }, + { + name: 'CONNECTION_REFUSED', + code: IO.CONNECTION_REFUSED, + reason: REASON.CONNECTION_REFUSED, + }, + { name: 'FAILED', code: IO.FAILED, reason: REASON.UNKNOWN }, + ])('reports $name as $reason', ({ code, reason }) => { + expect(reasonForIoError(code, IO, { local: true })).toBe(reason); + }); + }); }); diff --git a/tests/model.test.js b/tests/model.test.js index 5acc2d5..39679ed 100644 --- a/tests/model.test.js +++ b/tests/model.test.js @@ -998,6 +998,25 @@ describe('waiting files', () => { expect(daemon.deleted).toEqual([]); }); + // A full disk or an unwritable download directory is not the daemon's + // fault, and saying the daemon could not be reached sends someone to + // restart a service that is working. + it('reports a file it could not write here as a local failure', async () => { + const { model, daemon } = setup(); + await model.start(); + daemon.failures.set( + '/localapi/v0/files/a.txt', + new TransportError(REASON.LOCAL_FILE, 'No space left on device'), + ); + + const result = await model.saveFile('a.txt'); + + expect(Object.values(REASON)).toContain(result.error); + expect(result.error).toBe(REASON.LOCAL_FILE); + expect(daemon.deleted).toEqual([]); + expect(model.state.reachable).toBe(true); + }); + it('reports an unreadable list as nothing waiting', async () => { const { model, daemon } = setup(); await model.start(); diff --git a/tests/taildrop-section.test.js b/tests/taildrop-section.test.js index fcd1734..d7306a2 100644 --- a/tests/taildrop-section.test.js +++ b/tests/taildrop-section.test.js @@ -123,6 +123,31 @@ describe('received files', () => { expect(row.text).toMatch(/\S/); }); + // Not "Could not reach the Tailscale daemon": the daemon handed the file + // over, and it was the write here that failed. + it('says the file could not be saved when the write here fails', async () => { + const { panel, model, daemon } = setup(); + withFiles(daemon); + panel.enable(); + await model.start(); + await settle(); + toggleOf().menu.open(); + await settle(); + + daemon.failures.set('/localapi/v0/files/report.pdf', { + name: 'TransportError', + reason: REASON.LOCAL_FILE, + }); + + const row = toggleOf()._inbox.menu.items.at(0); + row.activate(); + await settle(); + + expect(row.text).toBe('Could not save the file.'); + expect(row.sensitive).toBe(true); + expect(daemon.deleted).toEqual([]); + }); + // modules/model.js's saveFile used to hand this row modules/errors.js's // already-composed English (messageFor), and _(error) then asked gettext // to translate a sentence it can never see when the .pot file is built — From fcb3943c7bf3defb3f60744a32a94dd2877d1bb5 Mon Sep 17 00:00:00 2001 From: napalm255 Date: Sat, 26 Sep 2026 20:56:48 -0400 Subject: [PATCH 02/19] fix(model): keep the daemon reachable when a change is refused One refused PATCH (an HTTP error or an unusable answer) went through #fail and marked the whole daemon unreachable: the subtitle said the daemon refused the request and the tile read off, over a menu that was otherwise working. It was also the only thing that set a flipped switch back, so a second refusal with the same reason changed nothing in the state and left the switch showing what the daemon refused. #patch now calls #fail only for SOCKET_MISSING, CONNECTION_REFUSED, UNKNOWN and PERMISSION_DENIED, the last so the operator row still shows. Any other reason is recorded with applyRefusal, which sets refusedReason and bumps refusedCount so that every refusal, a repeat included, is a change the panel hears about; _syncOptions then sets the switch back from the preferences that still hold, and the problems section says why. applyPrefs clears refusedReason. #patch resolves to { error } with the reason, or '' once applied. --- modules/model.js | 53 +++++++++++++++---- modules/panel.js | 16 +++++- modules/state.js | 32 ++++++++++++ tests/exit-node-section.test.js | 63 ++++++++++++++++++++++ tests/model.test.js | 79 +++++++++++++++++++++++++++- tests/panel.test.js | 93 +++++++++++++++++++++++++++++++++ tests/state.test.js | 56 ++++++++++++++++++++ 7 files changed, 378 insertions(+), 14 deletions(-) diff --git a/modules/model.js b/modules/model.js index 4ab5b2b..dbade4b 100644 --- a/modules/model.js +++ b/modules/model.js @@ -41,6 +41,7 @@ import { applyError, applyPrefs, applyProfiles, + applyRefusal, applyStatus, changed, initialState, @@ -56,6 +57,22 @@ import { backoffDelay, flushDelay } from './timing.js'; /** How long a peer list may go unread while the menu is closed. */ export const PEERS_STALE_MS = 60000; +/** + * The reasons a refused change still marks the daemon unreachable. + * + * Any other reason means the daemon answered: it refused the change, or said + * something unusable about it, and is plainly still there. PERMISSION_DENIED + * is answered too, but it is the missing operator, and the operator row that + * only an unreachable daemon shows is how anyone finds the command that fixes + * it. + */ +const UNREACHABLE = new Set([ + REASON.SOCKET_MISSING, + REASON.CONNECTION_REFUSED, + REASON.UNKNOWN, + REASON.PERMISSION_DENIED, +]); + /** The store. One per enable/disable lifetime. */ export class TailscaleModel { #client; @@ -385,27 +402,27 @@ export class TailscaleModel { // ---- commands --------------------------------------------------------- - /** @param {boolean} value Whether the tailnet should be up. @returns {Promise} Done. */ + /** @param {boolean} value Whether the tailnet should be up. @returns {Promise<{error: string}>} See #patch. */ setRunning(value) { return this.#patch({ WantRunning: Boolean(value) }); } - /** @param {boolean} value Accept subnet routes. @returns {Promise} Done. */ + /** @param {boolean} value Accept subnet routes. @returns {Promise<{error: string}>} See #patch. */ setAcceptRoutes(value) { return this.#patch({ RouteAll: Boolean(value) }); } - /** @param {boolean} value Use the tailnet's DNS. @returns {Promise} Done. */ + /** @param {boolean} value Use the tailnet's DNS. @returns {Promise<{error: string}>} See #patch. */ setAcceptDNS(value) { return this.#patch({ CorpDNS: Boolean(value) }); } - /** @param {boolean} value Reach the LAN while using an exit node. @returns {Promise} Done. */ + /** @param {boolean} value Reach the LAN while using an exit node. @returns {Promise<{error: string}>} See #patch. */ setAllowLanAccess(value) { return this.#patch({ ExitNodeAllowLANAccess: Boolean(value) }); } - /** @param {boolean} value Block incoming connections. @returns {Promise} Done. */ + /** @param {boolean} value Block incoming connections. @returns {Promise<{error: string}>} See #patch. */ setShieldsUp(value) { return this.#patch({ ShieldsUp: Boolean(value) }); } @@ -419,7 +436,7 @@ export class TailscaleModel { * withdraw a subnet this machine is routing for. * * @param {boolean} value Whether to advertise as an exit node. - * @returns {Promise} Done. + * @returns {Promise<{error: string}>} See #patch. */ setRunExitNode(value) { return this.#patch({ @@ -427,7 +444,7 @@ export class TailscaleModel { }); } - /** @param {boolean} value Run the Tailscale SSH server. @returns {Promise} Done. */ + /** @param {boolean} value Run the Tailscale SSH server. @returns {Promise<{error: string}>} See #patch. */ setSsh(value) { return this.#patch({ RunSSH: Boolean(value) }); } @@ -436,7 +453,7 @@ export class TailscaleModel { * Route through a peer, or stop doing so. * * @param {string} id Node id, or '' to use no exit node. - * @returns {Promise} Done. + * @returns {Promise<{error: string}>} See #patch. */ setExitNode(id) { return this.#patch({ ExitNodeID: id ?? '' }); @@ -497,17 +514,31 @@ export class TailscaleModel { * is also why a user-initiated change has no bus latency: the round trip * that applies it is the same one that reports it. * + * A change the daemon answered and refused leaves it reachable: one + * refused change is not evidence that the daemon has gone. It is recorded + * with applyRefusal instead, which is also what sets a switch the user + * flipped back to the preference that still holds. + * * @param {Record} changes Preferences to set. - * @returns {Promise} Done. + * @returns {Promise<{error: string}>} '' once applied, or canceled; + * otherwise the REASON it failed with. */ async #patch(changes) { - if (this.#disposed) return; + if (this.#disposed) return { error: '' }; try { const prefs = await this.#request(patchPrefsRequest(changes)); this.#commit(applyPrefs(this.#state, prefs)); + + return { error: '' }; } catch (error) { - this.#fail(error); + if (isCanceled(error)) return { error: '' }; + + const reason = reasonOf(error); + if (UNREACHABLE.has(reason)) this.#fail(error); + else this.#commit(applyRefusal(this.#state, reason)); + + return { error: reason }; } } diff --git a/modules/panel.js b/modules/panel.js index 20eca16..7aac15e 100644 --- a/modules/panel.js +++ b/modules/panel.js @@ -227,7 +227,10 @@ const QuickTSToggle = GObject.registerClass( // The switch reports what the user asked for; the daemon's // answer comes back through the model and is what finally - // sets the state. A refused change therefore reverts. + // sets the state. A refused change therefore reverts: the + // model counts every refusal, so even a repeat of the last + // one is a change, and _syncOptions sets the switch back from + // the preferences that still hold. item.connectObject( 'toggled', (_item, value) => void apply(value), @@ -262,7 +265,12 @@ const QuickTSToggle = GObject.registerClass( this._maybeOpenAuthUrl(state); - if (moved('reachable') || moved('errorReason') || moved('backendState')) + if ( + moved('reachable') || + moved('errorReason') || + moved('refusedReason') || + moved('backendState') + ) this._syncProblems(state); if (moved('health')) this._syncWarnings(state); @@ -326,6 +334,10 @@ const QuickTSToggle = GObject.registerClass( else item.setSensitive(false); this._problems.addMenuItem(item); + } else if (state.refusedReason) { + // A change the daemon answered and refused. The switch has + // already been set back by _syncOptions; this says why. + addDisabledRow(this._problems, problemMessage(state.refusedReason, _)); } if (needsLogin(state)) { diff --git a/modules/state.js b/modules/state.js index dfbc433..0f0eebe 100644 --- a/modules/state.js +++ b/modules/state.js @@ -28,6 +28,12 @@ export function initialState() { reachable: false, errorReason: '', + // Why the daemon, reachable, last refused a change, and how many + // times it has. The count is what makes a second identical refusal a + // change of its own; see applyRefusal(). + refusedReason: '', + refusedCount: 0, + // From /status. backendState: BACKEND.NO_STATE, authUrl: '', @@ -147,6 +153,8 @@ export function applyPrefs(state, prefs) { ...state, reachable: true, errorReason: '', + // The daemon's current answer; a refusal before it is no longer news. + refusedReason: '', running: prefs?.WantRunning === true, acceptRoutes: prefs?.RouteAll === true, @@ -220,6 +228,28 @@ export function applyError(state, reason) { }); } +/** + * Record that the daemon refused a change. + * + * It answered, so it is still reachable and everything already read still + * holds; the change just did not happen. The menu sets a flipped switch back + * from the unchanged preferences whenever it is told something moved, so the + * count moves on every refusal: a second one with the same reason would + * otherwise change nothing, tell nobody, and leave the switch showing what + * the daemon refused. + * + * @param {object} state Current state. + * @param {string} reason One of {@link REASON}. + * @returns {object} A new state. + */ +export function applyRefusal(state, reason) { + return derive({ + ...state, + refusedReason: reason || REASON.UNKNOWN, + refusedCount: state.refusedCount + 1, + }); +} + /** * Which fields differ between two snapshots. * @@ -253,6 +283,8 @@ export function changed(previous, next) { const SCALARS = Object.freeze([ ['reachable', s => s.reachable], ['errorReason', s => s.errorReason], + ['refusedReason', s => s.refusedReason], + ['refusedCount', s => s.refusedCount], ['backendState', s => s.backendState], ['authUrl', s => s.authUrl], ['magicDNSSuffix', s => s.magicDNSSuffix], diff --git a/tests/exit-node-section.test.js b/tests/exit-node-section.test.js index 87febe7..133fd4a 100644 --- a/tests/exit-node-section.test.js +++ b/tests/exit-node-section.test.js @@ -1,5 +1,6 @@ import { describe, expect, it } from 'vitest'; +import { REASON } from '../modules/errors.js'; import { KEYS } from '../modules/settings.js'; import { rawPeer, rawPeerMap, SUFFIX } from './fixtures/peers.js'; import * as Main from './stubs/shell-main.js'; @@ -101,6 +102,68 @@ describe('the exit node picker', () => { expect(daemon.patches.at(-1)).toMatchObject({ ExitNodeID: '' }); }); + // The row is ticked from the daemon's answer, not from the click, so a + // refused choice leaves the node actually in use ticked — and the menu + // says why rather than calling the daemon unreachable. + describe('a refused choice', () => { + const refuse = daemon => + daemon.failures.set('/localapi/v0/prefs', { + name: 'TransportError', + reason: REASON.HTTP, + }); + const rows = () => toggleOf()._exitNode.menu.items; + const choose = name => + rows() + .find(item => item.text === name) + .activate(); + + it('keeps the node in use ticked, and says why', async () => { + const { panel, model, daemon } = setup({ seed: withGateway() }); + panel.enable(); + await model.start(); + await settle(); + refuse(daemon); + + choose('gateway'); + await settle(); + + expect(daemon.patches.at(-1)).toMatchObject({ ExitNodeID: 'nGATE' }); + expect(rows().find(item => item.text === 'None').icon).toBe( + 'object-select-symbolic', + ); + expect(rows().find(item => item.text === 'gateway').icon).not.toBe( + 'object-select-symbolic', + ); + expect(toggleOf()._exitNode.label.text).toBe('Exit node'); + expect(model.state.reachable).toBe(true); + expect(labelsOf(toggleOf()._problems.items)).toEqual([ + 'The Tailscale daemon refused the request.', + ]); + }); + + it('still does after a second identical refusal', async () => { + const { panel, model, daemon } = setup({ seed: withGateway() }); + daemon.responses.prefs.ExitNodeID = 'nGATE'; + panel.enable(); + await model.start(); + await settle(); + refuse(daemon); + + choose('gateway'); + await settle(); + choose('None'); + await settle(); + + expect(daemon.patches).toHaveLength(2); + expect(rows().find(item => item.text === 'gateway').icon).toBe( + 'object-select-symbolic', + ); + expect(toggleOf()._exitNode.label.text).toBe('Exit node: gateway'); + expect(toggleOf().subtitle).toBe('via gateway'); + expect(model.state.reachable).toBe(true); + }); + }); + describe('Mullvad', () => { const mullvadPeer = (id, name, country, code, city) => rawPeer({ diff --git a/tests/model.test.js b/tests/model.test.js index 39679ed..028399e 100644 --- a/tests/model.test.js +++ b/tests/model.test.js @@ -262,6 +262,8 @@ describe('commands', () => { expect(model.state.errorReason).toBe(REASON.HTTP); }); + // A 403 is the missing operator, and the operator row is how anyone + // finds out, so it still marks the daemon unreachable. it('reports a failed command instead of throwing', async () => { const { model, daemon } = setup(); await model.start(); @@ -270,10 +272,85 @@ describe('commands', () => { new TransportError(REASON.PERMISSION_DENIED, '403'), ); - await expect(model.setRunning(false)).resolves.toBeUndefined(); + await expect(model.setRunning(false)).resolves.toEqual({ + error: REASON.PERMISSION_DENIED, + }); + expect(model.state.reachable).toBe(false); expect(model.state.errorReason).toBe(REASON.PERMISSION_DENIED); }); + it('reports an applied command with no error', async () => { + const { model } = setup(); + await model.start(); + + await expect(model.setShieldsUp(true)).resolves.toEqual({ error: '' }); + }); + + // The daemon answered and said no. One refused change is not evidence + // that the daemon has gone, and saying so would put "the daemon refused + // the request" in place of a menu that is otherwise working. + it.each([REASON.HTTP, REASON.PROTOCOL])( + 'keeps the daemon reachable when a change is refused with %s', + async reason => { + const { model, daemon } = setup(); + await model.start(); + daemon.failures.set('/localapi/v0/prefs', new TransportError(reason, 'no')); + + await expect(model.setShieldsUp(true)).resolves.toEqual({ error: reason }); + expect(model.state.reachable).toBe(true); + expect(model.state.errorReason).toBe(''); + expect(model.state.refusedReason).toBe(reason); + expect(model.state.shieldsUp).toBe(false); + }, + ); + + // What sets a flipped switch back is a change the menu hears about. The + // second refusal, identical to the first, has to be one too. + it('tells subscribers about every refusal, not just the first', async () => { + const { model, daemon } = setup(); + await model.start(); + daemon.failures.set( + '/localapi/v0/prefs', + new TransportError(REASON.HTTP, '500'), + ); + const listener = vi.fn(); + model.subscribe(listener); + + await model.setShieldsUp(true); + await model.setShieldsUp(true); + + expect(listener).toHaveBeenCalledTimes(2); + }); + + // A disable in the middle of a change is teardown, not a refusal. + it('reports nothing for a change canceled in flight', async () => { + const { model, daemon } = setup(); + await model.start(); + daemon.failures.set('/localapi/v0/prefs', new CanceledError()); + const listener = vi.fn(); + model.subscribe(listener); + + await expect(model.setShieldsUp(true)).resolves.toEqual({ error: '' }); + expect(listener).not.toHaveBeenCalled(); + expect(model.state.refusedReason).toBe(''); + }); + + it('clears the refusal once a change goes through', async () => { + const { model, daemon } = setup(); + await model.start(); + daemon.failures.set( + '/localapi/v0/prefs', + new TransportError(REASON.HTTP, '500'), + ); + await model.setShieldsUp(true); + + daemon.failures.clear(); + await model.setShieldsUp(true); + + expect(model.state.refusedReason).toBe(''); + expect(model.state.shieldsUp).toBe(true); + }); + it('reads status after a login so the auth URL can arrive', async () => { const { model, daemon } = setup(); await model.start(); diff --git a/tests/panel.test.js b/tests/panel.test.js index fd16cb7..c61b6cd 100644 --- a/tests/panel.test.js +++ b/tests/panel.test.js @@ -441,6 +441,78 @@ describe('the settings switches', () => { expect(rowsNamed(toggleOf(), 'Accept routes').at(0)).toBe(before); expect(before._wasDestroyed).toBe(false); }); + + // A switch moves the moment it is flipped, before the daemon has said + // anything. A refused change has to set it back, and say why, without + // calling a daemon that answered unreachable. + it('set themselves back when the daemon refuses the change', async () => { + const { panel, model, daemon } = setup(); + panel.enable(); + await model.start(); + await settle(); + const subtitle = toggleOf().subtitle; + daemon.failures.set('/localapi/v0/prefs', { + name: 'TransportError', + reason: REASON.HTTP, + }); + + const routes = rowsNamed(toggleOf(), 'Accept routes').at(0); + routes.toggle(); + await settle(); + + expect(routes.state).toBe(false); + expect(model.state.reachable).toBe(true); + expect(toggleOf().subtitle).toBe(subtitle); + expect(labelsOf(toggleOf()._problems.items)).toEqual([ + 'The Tailscale daemon refused the request.', + ]); + }); + + // The second refusal leaves every preference exactly as the first did. + // Only its being counted tells the menu to look again. + it('set themselves back after a second identical refusal', async () => { + const { panel, model, daemon } = setup(); + panel.enable(); + await model.start(); + await settle(); + daemon.failures.set('/localapi/v0/prefs', { + name: 'TransportError', + reason: REASON.HTTP, + }); + + const routes = rowsNamed(toggleOf(), 'Accept routes').at(0); + routes.toggle(); + await settle(); + routes.toggle(); + await settle(); + + expect(daemon.patches).toHaveLength(2); + expect(routes.state).toBe(false); + expect(labelsOf(toggleOf()._problems.items)).toEqual([ + 'The Tailscale daemon refused the request.', + ]); + }); + + it('drop the refusal once a change goes through', async () => { + const { panel, model, daemon } = setup(); + panel.enable(); + await model.start(); + await settle(); + daemon.failures.set('/localapi/v0/prefs', { + name: 'TransportError', + reason: REASON.HTTP, + }); + const routes = rowsNamed(toggleOf(), 'Accept routes').at(0); + routes.toggle(); + await settle(); + + daemon.failures.clear(); + routes.toggle(); + await settle(); + + expect(routes.state).toBe(true); + expect(toggleOf()._problems.items).toEqual([]); + }); }); describe('running as an exit node', () => { @@ -496,6 +568,27 @@ describe('running as an exit node', () => { expect(daemon.patches.at(-1).AdvertiseRoutes).toEqual(['192.168.1.0/24']); }); + + it('turns itself back off when the daemon refuses, every time', async () => { + const { panel, model, daemon } = setup(); + panel.enable(); + await model.start(); + await settle(); + daemon.failures.set('/localapi/v0/prefs', { + name: 'TransportError', + reason: REASON.HTTP, + }); + + const exitNode = rowsNamed(toggleOf(), 'Run as exit node').at(0); + exitNode.activate(); + await settle(); + expect(exitNode.state).toBe(false); + + exitNode.activate(); + await settle(); + expect(exitNode.state).toBe(false); + expect(model.state.reachable).toBe(true); + }); }); describe('profiles', () => { diff --git a/tests/state.test.js b/tests/state.test.js index 800349c..93969ff 100644 --- a/tests/state.test.js +++ b/tests/state.test.js @@ -6,6 +6,7 @@ import { applyError, applyPrefs, applyProfiles, + applyRefusal, applyStatus, changed, initialState, @@ -288,6 +289,61 @@ describe('applyError', () => { }); }); +describe('applyRefusal', () => { + // The daemon answered, so it is still there and what was read is still + // true. Only the change did not happen. + it('keeps the daemon reachable and what was already known', () => { + const loaded = applyPrefs(applyStatus(initialState(), status()), prefs()); + const refused = applyRefusal(loaded, REASON.HTTP); + + expect(refused.reachable).toBe(true); + expect(refused.errorReason).toBe(''); + expect(refused.running).toBe(true); + expect(refused.nodes).toHaveLength(1); + }); + + it('records why, and counts it', () => { + const refused = applyRefusal(initialState(), REASON.PROTOCOL); + + expect(refused.refusedReason).toBe(REASON.PROTOCOL); + expect(refused.refusedCount).toBe(initialState().refusedCount + 1); + }); + + it('falls back to unknown for an empty reason', () => { + expect(applyRefusal(initialState(), '').refusedReason).toBe(REASON.UNKNOWN); + }); + + it('starts with nothing refused', () => { + expect(initialState().refusedReason).toBe(''); + expect(initialState().refusedCount).toBe(0); + }); + + it('is frozen', () => { + expect(Object.isFrozen(applyRefusal(initialState(), REASON.HTTP))).toBe(true); + }); + + // A switch the user flipped is set back from the unchanged preferences + // only when a change tells the menu to look. Two refusals in a row with + // the same reason would otherwise look like nothing happened the second + // time, and leave the switch showing what the daemon refused. + it('registers a second identical refusal as a change', () => { + const once = applyRefusal(applyPrefs(initialState(), prefs()), REASON.HTTP); + const twice = applyRefusal(once, REASON.HTTP); + + expect(changed(once, twice)).toEqual(['refusedCount']); + }); + + // The next successful read or change is the daemon's current answer, and + // the refusal it follows is no longer news. + it('is cleared by the next preferences read', () => { + const refused = applyRefusal(applyPrefs(initialState(), prefs()), REASON.HTTP); + const read = applyPrefs(refused, prefs()); + + expect(read.refusedReason).toBe(''); + expect(changed(refused, read)).toEqual(['refusedReason']); + }); +}); + describe('changed', () => { it('reports nothing for identical snapshots', () => { const state = applyStatus(initialState(), status()); From 3639d6dbd9bab482c602dcaf0b9674839dbe137c Mon Sep 17 00:00:00 2001 From: napalm255 Date: Sat, 26 Sep 2026 21:00:45 -0400 Subject: [PATCH 03/19] fix(i18n): give translators the strings the preferences window shows prefs.js handed gettext variables: every settings row title and subtitle from SETTINGS' own English, and every routes failure as messageFor's composed sentence. xgettext only extracts literals, so none of them could ever reach the translation template, and the i18n check never saw it because it scanned modules/ only. Move problemMessage into import-free modules/errors.js, which prefs.js can reach, and re-export it from menu-items.js for the menu sections. Give modules/settings.js a settingText(key, _) of literal _() calls, leaving SETTINGS with the key and its type. The i18n check now also reads prefs.js and extension.js. messageFor and problemOf's message field had no reader left but their own tests, so both are gone. AGENTS.md now says what the i18n check scans, no longer claims Vitest runs prefs.js, and names settingText in the settings-key procedure. --- AGENTS.md | 38 ++++++++-------- modules/errors.js | 72 +++++++++++++++-------------- modules/health.js | 10 ++-- modules/menu-items.js | 39 ++-------------- modules/settings.js | 83 ++++++++++++++++++++++------------ prefs.js | 29 ++++-------- tests/device-section.test.js | 2 +- tests/errors.test.js | 36 +++++++++++---- tests/health.test.js | 13 ++---- tests/i18n.test.js | 11 +++-- tests/menu-items.test.js | 34 +++----------- tests/model.test.js | 3 +- tests/panel.test.js | 4 +- tests/settings.test.js | 39 ++++++++++++++-- tests/support/i18n.js | 66 +++++++++++++++++---------- tests/taildrop-section.test.js | 2 +- 16 files changed, 256 insertions(+), 225 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 9c78d49..ec2d2a0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,11 +81,12 @@ Run `just ci` before claiming anything done. already there, including a symlink — so nothing planted in the download directory is ever followed or overwritten. - **Every translatable string is a literal.** `tests/i18n.test.js` - (`tests/support/i18n.js`'s `nonLiteralGettextCalls`) fails if any `_()` - call's message, or either of `_n()`'s two arguments, is a variable, - property access or template rather than a string literal — invisible to - `xgettext -k_ -k_n:1,2` exactly the same way a translator would never - see it. + (`tests/support/i18n.js`'s `nonLiteralGettextCalls`) reads the source of + every file under `modules/`, `prefs.js` and `extension.js`, and fails if + any `_()` call's message, or either of `_n()`'s two arguments, is a + variable, property access or template rather than a string literal — + invisible to `xgettext -k_ -k_n:1,2` exactly the same way a translator + would never see it. - **No JavaScript on the docs pages.** `docs/index.html` ships no `