BACKLOG: document DC-033 (done) + add DC-034..041 from v1.14.8/0.0.0 incident
Captures the work done in this session (DC-033) and surfaces 9 follow-up items that came out of the cross-check investigation: P1: DC-034 (regenerate release tarball as 1.14.9), DC-035 (regression test for getLocalVersion), DC-036 (delete dead root self-updater.js), DC-037 (move symlink creation into install script so fresh hosts don't repeat the v1.14.4 failure mode). P2: DC-038 (backup trigger.json/result.json), DC-039 (audit for other __dirname antipatterns), DC-040 (audit whether post-deploy-patches.sh is still needed), DC-041 (integration test for the auto-update pipeline). Each ticket cites the specific files, commit SHAs, and evidence from this session so future agents can pick up where this left off.
This commit is contained in:
+68
@@ -148,6 +148,74 @@
|
||||
- **details:** `Logger.error()` in `src/utils/logging.js:256` calls `this._log('error', ...)` but does NOT return the result. `_log('error', ...)` returns the promise from `writeErrorLog(...)` (the async disk write to error.log). Because `error()` drops the return value, every `await logError(...)` / `await log.error(...)` caller is actually awaiting `undefined` — the file write becomes fire-and-forget. Symptoms: (1) `__tests__/logging.test.js` "captures request context when req is passed" fails intermittently in the full suite (passes in isolation) — the test reads error.log before the un-awaited appendFile completes. (2) In production, 6 route handlers (`routes/apps/deploy.js`, `routes/apps/removal.js`, `routes/health.js`, `routes/arr/config.js`, `routes/updates.js`) plus the global `boundAsyncHandler` error catcher all `await logError(...)` expecting the write to flush; error entries can be lost if the process exits/restarts immediately after. Latent since the original "unify logger" commit f71e5c5. Fix: add `return` to `Logger.error()` so the `writeErrorLog` promise propagates to callers. No behavior change for `debug/info/warn` (they never returned a promise and don't write to disk).
|
||||
- **result:** Fixed — one-line change (`return this._log(...)`). The logging flake is eliminated: **10/10 full-suite runs passed** (was ~1-in-6 failure rate before the fix). Production impact: every `await logError(...)` in route handlers and the global Express error catcher now actually waits for the error.log write to flush to disk, so error entries survive fast process exit/restart. No behavior change for debug/info/warn (they never wrote to disk). ESLint clean.
|
||||
|
||||
### DC-033: getLocalVersion() returns 0.0.0 — SelfUpdater uses __dirname but is loaded via ./src/docker/self-updater
|
||||
- **status:** done (commits 20d280f + 77536f4)
|
||||
- **owner:** krystie
|
||||
- **details:** Every DashCaddy host running v1.14.x (≤ v1.14.8) silently reports `version: 0.0.0, commit: null` from `/api/v1/system/version`, and `checkForUpdate()` always thinks we are outdated. Root cause: `server.js` lines 69 + 245 do `require('./src/docker/self-updater')`, so inside the container `__dirname` resolves to `/app/src/docker` which has no `package.json` or `VERSION` next to it. The function's outer `try/catch` swallows the `ENOENT` and returns the `{ version: '0.0.0', commit: null }` fallback. Discovered 2026-07-05 when DNS2 was running v1.14.4 (packaged from a pre-build-pipeline-fix tree that was already missing `src/`) and the dashboard showed 0.0.0 even though `/app/package.json` said 1.14.4. Confirmed by two independent investigations (main agent + z.ai subagent) reaching the same conclusion. Fix: rewrite `getLocalVersion()` to walk a candidate list — `path.join(__dirname, '..', '..', 'package.json')` first (the api root), then `path.join(__dirname, 'package.json')` (legacy root-copy contract). Add `console.error` on total failure instead of swallowing silently. Verified live on DNS2: `curl http://127.0.0.1:3001/api/v1/system/version` now returns `{"name":"DashCaddy","version":"1.14.8","commit":"20d280f"}`.
|
||||
- **result:** Done in two commits. (1) `20d280f DC-033: fix getLocalVersion __dirname resolution` — patched `src/docker/self-updater.js` `getLocalVersion()`. (2) `77536f4 DC-033: bump VERSION to 20d280f (DC-033 commit SHA)` — kept dashcaddy-api/VERSION in sync. Also restored DNS2 working tree to origin/main (was at v1.14.4 packaged from a stale tree; origin/main was at v1.14.8 with DC-020..032 security fixes intact — would have shipped as a downgrade if committed naively). Created `/etc/dashcaddy/sites/dashcaddy-api` → `/opt/dashcaddy/dashcaddy-api` symlink so future trigger.json `apiSourceDir` paths resolve correctly. Health: alive. /api/v1/system/version returns 1.14.8 (20d280f).
|
||||
|
||||
---
|
||||
|
||||
## P1 — Code Quality
|
||||
|
||||
### DC-034: Regenerate get.dashcaddy.net/release tarball as v1.14.9 with DC-033 baked in
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** Live `https://get.dashcaddy.net/release/version.json` advertises v1.14.8 (commit `ba23cdf`) but DC-033 is NOT in that tarball — verified by extracting `dashcaddy/dashcaddy-api/src/docker/self-updater.js` from `dashcaddy-1.14.8.tar.gz` and confirming it still has the broken `__dirname` pattern. Every other DashCaddy host that auto-updates to v1.14.8 will hit the same 0.0.0 dashboard bug DNS2 just had. Fix: (1) bump `package.json` to `1.14.9` + update `dashcaddy-api/VERSION` to the DC-033 commit SHA. (2) populate `[Unreleased]` section in CHANGELOG.md with DC-033 entry. (3) run `bash scripts/publish-release.sh` to rebuild + push the tarball to get.dashcaddy.net. (4) verify the live `version.json` reflects the new version + commit. Effort: ~15 min. Risk: low — release pipeline already proven by build-pipeline-fix.
|
||||
- **impact:** Every host auto-updating gets the 0.0.0 fix for free without needing a manual symlink or git pull.
|
||||
|
||||
### DC-035: Add regression test for getLocalVersion() — prevent DC-033 class from regressing
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** DC-033 fixed the bug but nothing in the test suite would have caught it originally. The existing coverage on `self-updater.js` is sparse — no test exercises `getLocalVersion()` directly. Add `__tests__/self-updater-version.test.js` that: (1) `require('./src/docker/self-updater')` (matching what server.js does, NOT `require('./self-updater')` which resolves from cwd and loads the wrong file — that's a separate footgun, see DC-036). (2) instantiate SelfUpdater with minimal config. (3) call `getLocalVersion()`. (4) assert `version` is NOT `'0.0.0'` and is in semver shape (`/^\d+\.\d+\.\d+/`). (5) assert `commit` matches `/^[0-9a-f]{7,40}$/`. Optionally: parameterize to also exercise `require('./self-updater')` from `/app` cwd to verify the legacy root-copy contract still works. Effort: ~20 min. Pattern: matches DC-017's depth-2-routes-smoke.test.js (loads every module via the real path).
|
||||
- **impact:** Catches the exact class of bug DC-033 fixed, plus any future refactor that re-introduces the __dirname antipattern.
|
||||
|
||||
### DC-036: Delete dead `dashcaddy-api/self-updater.js` (root copy) — 0 runtime callers
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** After DC-005 refactor (commit 283121e), there are TWO SelfUpdater implementations on disk: `/opt/dashcaddy/dashcaddy-api/self-updater.js` (md5 `79d566cc...`) and `/opt/dashcaddy/dashcaddy-api/src/docker/self-updater.js` (md5 `b3b61557...`). Both have drifted. **Zero runtime callers of the root copy** — verified by `grep -rn "require.*self-updater" dashcaddy-api/ --include="*.js"` which shows only `./src/docker/self-updater` (in server.js + src/app.js). The root copy is dead code from a prior refactor and a footgun for future contributors who edit the wrong file. Subagent flagged this independently. Fix: `git rm dashcaddy-api/self-updater.js` + verify `npx jest --passWithNoTests` still passes. Risk: very low. If a test does import it, the test itself is wrong and should be deleted or pointed at `./src/docker/self-updater`.
|
||||
- **impact:** Removes the wrong-file-edit footgun. Makes DC-035's test cleaner (only one SelfUpdater implementation to test).
|
||||
|
||||
### DC-037: Move `/etc/dashcaddy/sites/dashcaddy-api` symlink creation into the install script
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** DNS2 has the symlink manually created during this session (2026-07-05), but every other fresh DashCaddy install will hit the same `cp: cannot create directory '/etc/dashcaddy/sites/dashcaddy-api/routes': No such file or directory` failure when the first auto-update lands, because `dashcaddy-update.sh` defaults `apiSourceDir` to `${CADDY_BASE}/sites/dashcaddy-api` (= `/etc/dashcaddy/sites/dashcaddy-api`) while the actual install lives at `/opt/dashcaddy/dashcaddy-api`. Fix: add `mkdir -p /etc/dashcaddy/sites && ln -sfn /opt/dashcaddy/dashcaddy-api /etc/dashcaddy/sites/dashcaddy-api` to the install script (whichever of `dashcaddy-installer/install.sh` or `scripts/dashcaddy-install.sh` is canonical — verify which exists on a clean install). Make it idempotent (`ln -sfn`, not `ln -s`, so re-runs don't fail). Effort: ~10 min. Risk: very low.
|
||||
- **impact:** Prevents every future DashCaddy host from hitting the v1.14.4-class update failure on first auto-update.
|
||||
|
||||
---
|
||||
|
||||
## P2 — Polish & DX
|
||||
|
||||
### DC-038: Backup trigger.json + result.json in dashcaddy-update.sh — enable one-command rollback
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** During the DC-033 fix, recovering from the failed v1.14.4 update required manually mv'ing `trigger.json.processing` back to `trigger.json`, manually running `start.sh`, etc. — because the backup mechanism in `dashcaddy-update.sh` (lines 318-327) only backs up code + data, not the trigger/result state. Fix: in the `backup_data_dir` function (or new `backup_update_state` function), also copy `${UPDATES_DIR}/trigger.json` and `${UPDATES_DIR}/result.json` into the versioned backup directory so rollback tooling can restore them. Effort: ~15 min.
|
||||
- **impact:** Faster incident recovery. Currently takes 5-10 manual steps to roll back a failed update; would take 1.
|
||||
|
||||
### DC-039: Audit repo for other `__dirname + sibling-file` patterns — DC-033 class of bug
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** DC-033 was caused by `path.join(__dirname, 'package.json')` in a module loaded from a subdirectory. There may be other instances of the same pattern elsewhere in `src/`. Quick grep: `grep -rn "path.join(__dirname" dashcaddy-api/src/ --include="*.js"` and review each hit. Any that join `'package.json'`, `'VERSION'`, `'.env'`, `'openapi.yaml'`, `'Dockerfile'`, or `'.license-secret'` is suspect (these all live at the api root, not in subdirectories). For each suspect match, either: (a) verify the file does exist at the expected `__dirname` location, or (b) fix it to use the api-root path. Effort: ~30 min. Risk: low. Just an audit + targeted fixes.
|
||||
- **impact:** Catches latent bugs before users do. The fact that DC-033 shipped undiscovered through multiple releases suggests this antipattern might exist elsewhere.
|
||||
|
||||
### DC-040: Investigate whether dashcaddy-post-deploy-patches.sh is still needed at all
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** The script applies 23+ `require()` path fixes on every update (audit from `BUILD-PIPELINE-FIX.md` shows it was created to paper over `dashcaddy-api/src/` being missing from tarballs). After the build-pipeline-fix (which now ships `src/` in every tarball), most of those patches should be no-ops. If any are still applying real changes, that means the source tree has a latent bug that DC-005-era refactors missed. Run `bash scripts/dashcaddy-post-deploy-patches.sh` against a fresh checkout of origin/main (or extract the v1.14.8 tarball to a clean dir) and count how many patches actually change anything vs are no-ops. If most are no-ops, the script can either be deleted entirely (cleanest) or kept as a defensive backstop with a comment explaining its purpose has shifted to "verify src/ shipped correctly." Effort: ~45 min. Risk: medium — safer to keep as backstop with reduced scope.
|
||||
- **impact:** Clarity. The current state — "script applies 23 fixes every update but only 3-4 actually do anything" — is opaque and brittle.
|
||||
|
||||
### DC-041: Add integration test for the auto-update pipeline (trigger.json → bash → docker rebuild → health check → result.json)
|
||||
- **status:** todo
|
||||
- **owner:** unclaimed
|
||||
- **details:** The host-side updater has zero integration coverage. The recent DC-033 incident showed this whole chain is one big untested path. Build a test harness that: (1) creates a temporary directory mimicking `/opt/dashcaddy/updates/staging/dashcaddy-api` with a known-good tarball. (2) writes a `trigger.json` to a test `UPDATES_DIR`. (3) runs `bash /opt/dashcaddy/scripts/dashcaddy-update.sh` with paths overridden via env vars. (4) asserts `result.json` has `success: true` and the version matches. (5) cleans up. Effort: ~2 hours. Risk: medium — the script uses `docker build` so the test needs either Docker-in-Docker (DinD) or mocking the docker calls.
|
||||
- **impact:** Closes the biggest untested surface in DashCaddy. Would have caught the v1.14.4 packaging bug immediately on the next release.
|
||||
|
||||
---
|
||||
|
||||
## Backlog note (2026-07-05)
|
||||
|
||||
Tickets DC-033 through DC-041 were added after the DNS2 v1.14.4 / v1.14.8 / 0.0.0 incident. They are grounded in real evidence from that session — see DC-033's details for the full chain of reasoning (cross-checked by main agent + z.ai subagent).
|
||||
|
||||
## Coordination Rules
|
||||
|
||||
1. **Always `git pull` before starting work.**
|
||||
|
||||
Reference in New Issue
Block a user