fix(main): batch-done race could orphan a freshly-spawned UploadManager

The batch-done event handler awaits configStore.appendHistory(summary)
before nulling the global uploadManager reference. If the renderer
fires start-upload while that await is pending, the start-upload IPC
creates a fresh UploadManager and assigns it to the same global. The
old handler resumes, sets uploadManager = null, and orphans the new
manager: cancel-upload, add-jobs-to-batch, save-config re-resolve etc.
all see null and become no-ops, while the new batch keeps running
invisibly in the background.

Capture the manager identity at listener registration time and only
null the global if it still points at THIS manager. If a newer one
replaced it mid-await, leave it alone and log the near-miss for
diagnostics.

Found by deep-audit subagent. Tests still 119/119 (no test for this
because it needs a coordinated IPC + async-mock harness; the fix is
small and the diagnostic log will catch regressions).
This commit is contained in:
Administrator 2026-04-28 09:12:48 +02:00
parent 794e4162e1
commit 04e535c709
2 changed files with 16 additions and 1 deletions

10
main.js
View File

@ -1270,6 +1270,13 @@ ipcMain.handle('start-upload', (_event, payload) => {
}
});
// Capture the manager identity at listener-registration time so the post-
// batch null-out can compare against IT — not against whatever the global
// happens to point at after an `await`. Without this, a renderer that
// fires start-upload while we're still awaiting appendHistory would
// create a fresh manager which the trailing `uploadManager = null` then
// orphans (cancel/addJobs see null, the new batch keeps running invisibly).
const _thisManager = uploadManager;
uploadManager.on('batch-done', async (summary) => {
debugLog(`batch-done: total=${summary.total} ok=${summary.succeeded} fail=${summary.failed}`);
logMemorySnapshot('batch-done');
@ -1282,7 +1289,8 @@ ipcMain.handle('start-upload', (_event, payload) => {
// Shutdown after finish
handleShutdownAfterFinish();
uploadManager = null;
if (uploadManager === _thisManager) uploadManager = null;
else debugLog('batch-done: skipping uploadManager null-out — a newer manager replaced this one mid-await');
});
// Defer startBatch to next tick so the IPC response is sent first.

View File

@ -13,9 +13,16 @@
- ✅ 3.3.9 — Throttled-Cache nach `lib/throttled-cache.js` extrahiert (von sortQueueJobs dynamic-throttle genutzt) + 12 Unit-Tests (TTL-Boundary, identity-tracking, fake-clock, peek/clear, refreshMs=0, large-input)
- ✅ 3.3.10 — `npm audit fix` (non-breaking): 4 vulnerabilities geschlossen (16 → 12), nur Lock-file Update
- ✅ 3.3.11 — Patch-Bumps `eslint 10.1→10.2`, `undici 7.24→7.25`, `ws 8.19→8.20` (semver-compatible)
- ✅ 3.3.12 — Race condition fix: `uploadManager = null` in batch-done clobberte einen frisch gespawnten Manager wenn user mid-await neuen batch startete (deep-audit finding HIGH-1)
## Open items (priorisiert)
### Stabilität (neu aus deep-audit)
- [ ] `save-global-settings-sync` swallows write errors silently (main.js:1612-1628) — returns true even on disk-full / AV-lock. Should set returnValue=false + debugLog.
- [ ] `parseByseResult` / `parseDoodstreamResult` crash on null payload (lib/hosters.js:202, 214, 163, 491) — `JSON.parse('null')` returns null, then `payload.files` throws TypeError. Guard `if (!payload || typeof payload !== 'object') throw...` after JSON.parse.
- [ ] Cancellation latency in retry-loop's account-rotation block (lib/upload-manager.js:680-792) — re-check signal.aborted after each await.
- [ ] TOCTOU on backup write in `_atomicWrite` (lib/config-store.js:254-258) — readFileSync after existsSync without try/catch; can fail if AV briefly locks file.
### Code-Qualität (deferred)
- [ ] removeFromQueueOnDone microtask-coalesce (3.3.1) — Microtask-Timing schwer zu testen ohne fake-timer setup
- [ ] 12 weitere Vulnerabilities (10 high, 2 low) in electron-builder dev-chain — bräuchten `npm audit fix --force` mit Major-Bump electron-builder@26.8.1 (breaking). Skip bis User explizit ein Major-Update erlaubt.