fix(launcher): address code review findings in stats sync

- merge-catalog: stop dropping anilist_id on every new anime insert
  (bun:sqlite .get() returns null, so the old !== undefined guard always
  fired); the guard was dead anyway since a colliding id is caught earlier
- sync-command: throw instead of fail() inside runHostSync so the finally
  block runs and temp dirs holding snapshot data are cleaned up on failure
- ssh: shell-quote the user-supplied --remote-cmd in the probe, and reject
  option-like (-prefixed) SSH hosts that ssh/scp would parse as flags
- sync-db: close the remote handle if opening the local DB throws
- sync-shared: treat EPERM from process.kill(pid, 0) as alive, not dead
- cli-parser: trim the sync host before the mode-exclusivity check
- tests: cover anilist_id preservation, anilist-based anime matching, and
  the SSH host/shell-quote guards
This commit is contained in:
2026-07-08 23:51:46 -07:00
parent be31f96f02
commit 58d54311d8
8 changed files with 131 additions and 21 deletions
+3 -9
View File
@@ -138,15 +138,9 @@ export function mergeAnime(
);
continue;
}
// anilist_id is UNIQUE; drop it if another local row (matched by a
// different title key) already claims it.
const anilistTaken =
row.anilist_id !== null && byAnilist.get(row.anilist_id) !== undefined
? null
: row.anilist_id;
const values = ANIME_COPY_COLUMNS.map((column) =>
column === 'anilist_id' ? anilistTaken : row[column],
);
// No local row matched by anilist_id (checked first in `existing` above)
// or title key, so the remote anilist_id — if any — is free to insert as-is.
const values = ANIME_COPY_COLUMNS.map((column) => row[column]);
map.set(remoteId, insertRow(local, 'imm_anime', ANIME_COPY_COLUMNS, values));
summary.animeAdded += 1;
}
+19
View File
@@ -0,0 +1,19 @@
import test from 'node:test';
import assert from 'node:assert/strict';
import { assertSafeSshHost, shellQuote } from './ssh.js';
test('assertSafeSshHost rejects option-like hosts', () => {
assert.throws(() => assertSafeSshHost('-oProxyCommand=touch pwned'), /looks like an option/);
assert.throws(() => assertSafeSshHost('-lroot'), /looks like an option/);
});
test('assertSafeSshHost accepts normal destinations', () => {
assert.doesNotThrow(() => assertSafeSshHost('macbook'));
assert.doesNotThrow(() => assertSafeSshHost('user@192.168.1.20'));
assert.doesNotThrow(() => assertSafeSshHost('ssh-alias'));
});
test('shellQuote escapes single quotes and wraps in quotes', () => {
assert.equal(shellQuote('subminer'), `'subminer'`);
assert.equal(shellQuote(`a'; rm -rf ~; '`), `'a'\\''; rm -rf ~; '\\'''`);
});
+22 -3
View File
@@ -5,12 +5,24 @@ export interface RemoteRunResult {
stdout: string;
}
/**
* ssh/scp have no `--` terminator for the destination, so a host that starts
* with `-` (e.g. `-oProxyCommand=...`) is parsed as an option. Reject those
* before spawning.
*/
export function assertSafeSshHost(host: string): void {
if (host.startsWith('-')) {
throw new Error(`Refusing to use SSH host that looks like an option: ${host}`);
}
}
/**
* Run a command on the SSH host. stdin/stderr stay attached to the terminal
* so key passphrase / password prompts and remote progress output work;
* stdout is captured for the caller.
*/
export function runSsh(host: string, remoteCommand: string): RemoteRunResult {
assertSafeSshHost(host);
const result = spawnSync('ssh', [host, remoteCommand], {
encoding: 'utf8',
stdio: ['inherit', 'pipe', 'inherit'],
@@ -43,11 +55,18 @@ export function shellQuote(value: string): string {
* configured command first and fall back to the default install location.
*/
export function resolveRemoteSubminerCommand(host: string, preferred: string | null): string {
const candidates = preferred ? [preferred] : ['subminer', '~/.local/bin/subminer'];
// Trusted defaults stay unquoted so the remote shell expands `~`; the
// user-supplied override is shell-quoted to prevent command injection.
const candidates: Array<{ value: string; probe: string }> = preferred
? [{ value: preferred, probe: shellQuote(preferred) }]
: [
{ value: 'subminer', probe: 'subminer' },
{ value: '~/.local/bin/subminer', probe: '~/.local/bin/subminer' },
];
for (const candidate of candidates) {
const probe = runSsh(host, `command -v ${candidate} >/dev/null 2>&1`);
const probe = runSsh(host, `command -v ${candidate.probe} >/dev/null 2>&1`);
if (probe.status === 0) {
return candidate;
return candidate.value;
}
}
throw new Error(
+59
View File
@@ -151,6 +151,65 @@ test('is idempotent: re-merging the same snapshot changes nothing', () => {
}
});
test('preserves anilist_id when inserting a new anime from the snapshot', () => {
const { dir, localPath, remotePath } = makeDbPair();
try {
insertFixtureSession(remotePath, {
uuid: 'remote-1',
videoKey: 'showb-e1',
animeTitleKey: 'showb',
startedAtMs: BASE_MS,
applyLifetime: true,
});
const remoteDb = new Database(remotePath, { readwrite: true });
remoteDb.prepare(`UPDATE imm_anime SET anilist_id = 12345 WHERE normalized_title_key = 'showb'`).run();
remoteDb.close();
const summary = mergeSnapshotIntoDb(localPath, remotePath);
assert.equal(summary.animeAdded, 1);
const anime = queryOne<{ anilist_id: number }>(
localPath,
`SELECT anilist_id FROM imm_anime WHERE normalized_title_key = 'showb'`,
);
assert.equal(anime?.anilist_id, 12345);
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
});
test('matches an existing local anime by anilist_id even when the title key differs', () => {
const { dir, localPath, remotePath } = makeDbPair();
try {
insertFixtureSession(localPath, {
uuid: 'local-1',
videoKey: 'showa-e1',
animeTitleKey: 'show-romaji',
startedAtMs: BASE_MS,
applyLifetime: true,
});
insertFixtureSession(remotePath, {
uuid: 'remote-1',
videoKey: 'showa-e2',
animeTitleKey: 'show-native',
startedAtMs: BASE_MS + DAY_MS,
applyLifetime: true,
});
const localDb = new Database(localPath, { readwrite: true });
localDb.prepare(`UPDATE imm_anime SET anilist_id = 999 WHERE normalized_title_key = 'show-romaji'`).run();
localDb.close();
const remoteDb = new Database(remotePath, { readwrite: true });
remoteDb.prepare(`UPDATE imm_anime SET anilist_id = 999 WHERE normalized_title_key = 'show-native'`).run();
remoteDb.close();
const summary = mergeSnapshotIntoDb(localPath, remotePath);
// Same anilist_id → one anime, not two.
assert.equal(summary.animeAdded, 0);
assert.equal(count(localPath, 'SELECT COUNT(*) AS n FROM imm_anime'), 1);
} finally {
fs.rmSync(dir, { recursive: true, force: true });
}
});
test('matches shared videos by video_key and merges the watched flag', () => {
const { dir, localPath, remotePath } = makeDbPair();
try {
+7 -1
View File
@@ -35,7 +35,13 @@ export function mergeSnapshotIntoDb(localDbPath: string, snapshotPath: string):
}
const remote = new Database(snapshotPath, { readonly: true });
const local = new Database(localDbPath, { readwrite: true, create: false });
let local: Database;
try {
local = new Database(localDbPath, { readwrite: true, create: false });
} catch (error) {
remote.close();
throw error;
}
try {
assertMergeableSchema(remote, 'Snapshot');
assertMergeableSchema(local, 'Local');
+4 -2
View File
@@ -121,8 +121,10 @@ function isProcessAlive(pid: number): boolean {
try {
process.kill(pid, 0);
return true;
} catch {
return false;
} catch (error) {
// EPERM means the process exists but we can't signal it → still alive.
// Only ESRCH (no such process) means it's actually gone.
return (error as NodeJS.ErrnoException)?.code === 'EPERM';
}
}