OpenVelo Agent
31 lines
4.4 KiB
Markdown
31 lines
4.4 KiB
Markdown
# Implementation Plan: Job 871 — follow-up tweaks
|
|
|
|
## 1. Architectural Reconnaissance
|
|
- The previous implementation introduced a `RestoreService` that takes a `ConfigService` constructor parameter but does not retain it on `this`. Internally `stageArchive()` calls `resolveSystemStagingDir(this.requireConfig())`, where `requireConfig()` returns the parameter via a cast `(this as any).configService`. This works but is a hack — the parameter should simply be retained as a class field.
|
|
- The previous `DangerZoneService.wipeChallenges()` snapshots `<UPLOAD_DIR>/challenges` to a side directory, transactionally deletes the DB rows, and then removes the snapshot. It currently never actually deletes the live `<UPLOAD_DIR>/challenges` directory on success, so the filesystem files persist even though the DB rows are gone. The user wants the live directory physically removed after the DB commit succeeds, with the snapshot retained as the rollback source until the disk delete is complete so that a disk-delete failure can be restored from it.
|
|
|
|
## 2. Impacted Files
|
|
- **To Modify:**
|
|
- `backend/src/modules/admin/system/restore.service.ts` — retain `ConfigService` on the instance and stop using the `requireConfig()` shim; use it directly in `stageArchive()`.
|
|
- `backend/src/modules/admin/system/danger-zone.service.ts` — in `wipeChallenges()`, after the DB transaction commits, physically remove the live `<UPLOAD_DIR>/challenges` directory, and only after that succeeds remove the snapshot; on any failure after the snapshot was created, copy the snapshot back into place so a disk-delete failure can be reversed.
|
|
|
|
## 3. Proposed Changes
|
|
1. **`restore.service.ts` cleanup:**
|
|
- Convert the constructor's `config: ConfigService` parameter into `private readonly configService: ConfigService` so it is stored on the instance.
|
|
- At the top of the constructor body, capture each derived value (`uploadDir`, `databasePath`, `stageTtlMs`, `restoreUploadLimit`) from `configService` once.
|
|
- Remove the private `requireConfig()` shim method entirely.
|
|
- In `stageArchive()`, change the call to `resolveSystemStagingDir(this.configService)`.
|
|
2. **`danger-zone.service.ts` physical delete:**
|
|
- In `wipeChallenges()`, after the `dataSource.transaction(...)` block returns successfully (we already have the row counts in `counts`), add: `FilesystemTransactionService.rmSafe(challengesDir)` to physically remove the live `<UPLOAD_DIR>/challenges` directory and its contents.
|
|
- If that rm succeeds, then `rmSafe` the `stagingBackup` snapshot.
|
|
- If the rm of `challengesDir` itself throws, retain the snapshot so the catch branch can restore from it (keep `staged = true`), rethrow the error to enter the catch branch, and from the existing catch branch copy `stagingBackup` back into `challengesDir` exactly as before. Update the in-method narrative so that order of operations matches: stage → db commit → live rm → snapshot rm.
|
|
- Do not change the DB ordering: the transaction must commit before any filesystem delete so a DB failure does not leave a partial filesystem state.
|
|
|
|
## 4. Test Strategy
|
|
- **Target Unit Test File:**
|
|
- Existing `tests/backend/admin-system-danger.spec.ts` already verifies that after `wipe-challenges` the `challenge` table is empty. Extend it with one assertion: after a successful wipe, the live `<UPLOAD_DIR>/challenges` directory is also physically empty/absent.
|
|
- Existing `tests/backend/admin-system-backup.spec.ts` and `tests/backend/admin-system-restore-validation.spec.ts` already validate `BackupService` and `RestoreService.stageArchive()` behavior; no behavioral changes are expected from the constructor cleanup since `stageArchive()` still resolves the same staging directory. Re-run those tests after the edit to confirm no regressions.
|
|
- No new frontend tests are required — these are strictly backend concerns.
|
|
- **Mocking Strategy:** Reuse the existing per-suite isolated temp upload directories (`mkdtempSync(path.join(os.tmpdir(), 'hipctf-…'))`) and assert against the live path with `fs.existsSync`/`fs.readdirSync`. No new mocking infrastructure is needed; the danger-zone logic now relies on the same primitives (fs + SQLite transaction) the existing tests already exercise.
|
|
- **Verification:** Run `npm test` to confirm both `admin-system-danger.spec.ts` and the related system suites stay green; also run `npx --workspace backend tsc -p tsconfig.build.json --noEmit` to confirm the `requireConfig()` removal does not break the build.
|