archy/.planning/codebase/CONCERNS.md
2026-07-29 10:25:54 -04:00

196 lines
18 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Codebase Concerns
**Analysis Date:** 2026-07-29
## Tech Debt
**Federation node removal tombstone gap:**
- Issue: `federation::remove_node()` (`core/archipelago/src/federation/storage.rs:180-197`) calls `tombstone_did()` at line 193 but explicitly drops the error with `let _ = …`. If tombstone write fails (disk I/O, permission, transient), the peer is removed from `nodes.json` but never actually recorded as removed, so the next background sync/notify-join silently re-adds it.
- Files: `core/archipelago/src/federation/storage.rs:180-197`, `core/archipelago/src/api/rpc/federation/handlers.rs:272-300`
- Impact: Federation peers marked for removal can reappear after the next sync cycle, confusing the operator and potentially re-establishing unwanted connections.
- Fix approach: Surface the tombstone-write failure instead of swallowing it; consider retry logic with backoff; add integration test via `tests/multinode/smoke.sh` to verify removal sticks across sync cycles.
**Container reconciler observability gap:**
- Issue: No metrics distinguish "settling after restart" from "flapping" — container thrashing is invisible until anecdotal reports. No per-app restart counter or log line when an app restarts >N times in M minutes.
- Files: `core/archipelago/src/container/prod_orchestrator.rs` (reconciler loop), `core/archipelago/src/health_monitor.rs`
- Impact: Silent restart storms go unnoticed; users see frequent service interruptions without diagnostics; operator can't distinguish normal convergence from a crash loop.
- Fix approach: Add per-app restart counter + log line when threshold exceeded; emit metric on each restart; wire restart count into health/status RPC output.
**Failed systemd unit self-healing gap:**
- Issue: When a Quadlet-backed app's `.service` unit enters `failed` state (e.g., exit 255), the reconciler does not automatically `reset-failed` + `start` it. The unit sits failed until the operator manually intervenes or the service restarts.
- Files: `core/archipelago/src/container/prod_orchestrator.rs` (reconcile loop)
- Impact: Apps with transient failures go down and stay down; no automatic recovery; operator must manually reset or restart the orchestrator.
- Fix approach: Add reconcile step: quadlet-backed app whose `.service` is `failed` and not user-stopped → call `systemctl --user reset-failed <unit>` + `start`; add backoff to avoid busy-loop on persistent failures.
**Bitcoin RPC credentials not retrieved from config/secrets:**
- Issue: `core/container/src/bitcoin_simulator.rs:158` has a TODO marking hardcoded (or missing) RPC credentials in the Bitcoin simulator real-mode path. Credentials should be fetched from the secret store.
- Files: `core/container/src/bitcoin_simulator.rs:155-165`
- Impact: Bitcoin simulator in real mode (Testnet/Mainnet) cannot authenticate to the node; RPC calls fail.
- Fix approach: Inject `SecretsProvider` into `BitcoinSimulator::new()` or pass credentials as constructor args; fetch via `config/secrets` at runtime; handle credential rotation.
**Container security policies not wired in:**
- Issue: `core/security/src/container_policies.rs` generates AppArmor/SELinux profiles but the `apply_profile()` function has a TODO at line 71: "Configure Podman to use the profile" — the profiles are generated but never applied to running containers.
- Files: `core/security/src/container_policies.rs:63-75`
- Impact: Security profiles exist but provide zero protection; containers run without the intended isolation constraints.
- Fix approach: Pass `--security-opt apparmor=<profile>` (or SELinux equivalent) to Podman at container creation; verify profile loads via `apparmor_status`; add CI check that profiles compile cleanly.
**Dynamic resource adjustment not implemented:**
- Issue: `core/performance/src/resource_manager.rs:86` has a TODO for dynamic resource adjustment based on usage. The allocator is static; no adaptive rebalancing when load patterns shift.
- Files: `core/performance/src/resource_manager.rs:86-88`
- Impact: Resource allocation is rigid; a node with skewed usage (e.g., one app consuming all memory) has no mechanism to rebalance dynamically.
- Fix approach: Monitor per-app resource usage via cgroup stats; implement feedback loop to adjust limits; gate on production deployment (likely Phase 3+).
## Known Bugs
**Multinode RPC robustness gap:**
- Symptoms: The `node_rpc()` function in `tests/multinode/lib/multinode.bash` lacks `--max-time` on curl calls — a slow server-side RPC can hang the test suite indefinitely with zero feedback.
- Files: `tests/multinode/lib/multinode.bash` (exact line TBD; see grep for `node_rpc`)
- Trigger: Run multinode federation/mesh test against a slow or overloaded node; curl will block forever.
- Workaround: Manually kill the test process and diagnose the hanging RPC manually; no automatic timeout recovery.
- Fix approach: Add `--max-time 30` to all curl calls in `node_rpc()`; re-run `tests/multinode/smoke.sh` to verify.
## Security Considerations
**Secrets environment variable exposure risk:**
- Risk: Bitcoin and other service credentials are materialized as env vars in `ARCHIPELAGO_*` (e.g., `BITCOIN_RPC_PASSWORD`). Env vars are visible via `/proc/<pid>/environ` and potentially logged.
- Files: `core/archipelago/src/container/prod_orchestrator.rs`, `core/container/src/manifest.rs`, `core/archipelago/src/api/rpc/package/config.rs`
- Current mitigation: Secrets are declared as `generated_secrets` in manifests and materialized 0600/rootless; the orchestrator avoids logging values.
- Recommendations: Audit all env-var passing to containers; consider switching high-sensitivity secrets (bitcoin RPC, LND macaroons) to file-based secrets mounted read-only; add audit logging for secret access.
**Federation DID validation incomplete:**
- Risk: Federation peer DIDs are added via the RPC without cryptographic verification of ownership. A compromised peer could advertise arbitrary DIDs.
- Files: `core/archipelago/src/api/rpc/federation/handlers.rs` (add-node path), `core/archipelago/src/federation/storage.rs`
- Current mitigation: DIDs are stored locally; transitive federation discovery uses the tombstone list to block removed peers.
- Recommendations: Add DID-ownership proof (e.g., signed proof-of-identity) before accepting a peer's advertised DID; document the trust model; consider user warnings when adding peers.
**AppArmor profiles overly permissive:**
- Risk: Generated AppArmor profiles use blanket `network,` instead of per-port/protocol rules. Readonly flag is checkbox only, not enforced per actual app needs.
- Files: `core/security/src/container_policies.rs:46-54`
- Current mitigation: None (profiles not applied).
- Recommendations: Refine per-app capabilities based on manifest's declared needs; add integration test verifying readonly mounts are enforced; apply profiles in development before prod.
## Performance Bottlenecks
**Container thrashing during reconcile:**
- Problem: Restarting `archipelago.service` SIGKILLs every container, forcing a full rebuild over several minutes. Uninstall + reinstall loops can cascade-trigger restarts.
- Files: `core/archipelago/src/container/prod_orchestrator.rs` (the reconciler's desired-state machine)
- Cause: Pre-Phase-3 architecture: containers run in systemd cgroup, not as independent Quadlet units.
- Improvement path: Phase-3 Quadlet default-flip (`config.rs:256`) — each app becomes an independent `.container` unit; restart only the affected app, not the entire cgroup.
**Reconciler churn on boot:**
- Problem: Boot reconciler makes multiple passes reconciling drift; during each pass, containers may be recreated. Post-OTA health checks deliberately skip per-app container assertions because of restart-storm unpredictability.
- Files: `core/archipelago/src/container/prod_orchestrator.rs`, `core/archipelago/src/bootstrap.rs`
- Cause: Multi-pass reconciliation + no incremental diff detection.
- Improvement path: Consolidate reconciler into single pass for boot; cache manifest/config diffs to avoid redundant comparisons; add boot-only fast-path.
**Bitcoin IBD on .198 stalled (disk I/O):**
- Problem: .198 bitcoin is mid-IBD with only 21% progress; disk is 448GB (below 1TB archival threshold); load is high (~35).
- Files: `tests/multinode-testing-plan.md` (documented issue)
- Cause: Undersized/slow disk; concurrent workload.
- Improvement path: User decision required: swap in a different node (already done for gate run, using .5 instead) or add storage + wait for sync. Not a code issue.
## Fragile Areas
**Uninstall + reinstall lifecycle:**
- Files: `core/archipelago/src/api/rpc/package/install.rs`, `core/archipelago/src/container/quadlet.rs:disable_remove()`, `neode-ui/src/components/AppCard.vue`
- Why fragile: Pre-2026-07-26, `quadlet::disable_remove()` called systemd + podman with no timeouts, causing hangs. Fixed by commit `71cc9ac4` (added `QUADLET_STOP_TIMEOUT`, SIGKILL escalation, reset-failed). AppCard was hardcoding uninstall bar to "stuck full-red" (fixed `9f17ba68`). Tests for reinstall/cascade are still opt-in.
- Safe modification: Any changes to the uninstall path must be tested via `cascade-uninstall.bats` (7/7 on .228); extend coverage to multi-container stacks (immich, btcpay). Verify on .228 before fleet roll.
- Test coverage: `tests/lifecycle/bats/cascade-uninstall.bats` exists but not in canonical gate; must opt-in with `ARCHY_GATE_CASCADE=1`.
**Production orchestrator state machine:**
- Files: `core/archipelago/src/container/prod_orchestrator.rs` (6291 lines)
- Why fragile: Largest file in the codebase; owns install/start/stop/restart/remove/upgrade for every app; per-app mutex + RwLock concurrency model; complex dependency resolution, adoption scan, Quadlet rendering, and host-port-wait logic interleaved.
- Safe modification: Understand the per-app mutex protocol before touching state mutation; test all changes via the lifecycle gate on .228; use the adoption scan + manifest merge logic for any new manifest evolution.
- Test coverage: 667 unit tests green (2026-07-01); lifecycle gate covers ~8 core apps; ~30 apps untested in gate.
**Mesh radio configuration + boot race:**
- Files: `core/archipelago/src/mesh/meshtastic.rs`, `core/archipelago/src/mesh/mod.rs`, tests at `tests/lifecycle/bats/meshtastic.bats`
- Why fragile: Radio boot-race fixed (2026-07-28, `a8c4694c`/`3f76b496`); on-air config apply must finish before device is used. Earlier versions had probe-boot-race + live config propagation issues. Must verify on real hardware.
- Safe modification: Any mesh changes require E2E test on real LoRa radios (dev-box ↔ x250-dev, or fleet broadcast); unit tests alone won't catch RF timing issues.
- Test coverage: 8-stage on-air smoke test in `tests/multinode/meshtastic.sh` (run manually; not in canonical gate).
**Lightning payment state machine:**
- Files: `core/archipelago/src/api/rpc/lnd/wallet.rs:payinvoice()`
- Why fragile: Slow multi-hop payments (>15s) previously surfaced as "failed" while settling in background; client-side 15s timeout was aborting the wait. Fixed by commit `614a0f5a` (120s wait, pending status, lnd.paymentstatus poll). Must verify on Framework PT with real multi-hop.
- Safe modification: Any lnd state changes must test full payment lifecycle: invoice creation, encoding, send, multi-hop wait, settlement confirmation. Verify on Framework PT before release.
- Test coverage: Local LND payinvoice smoke test; no multinode lightning routing test in gate.
## Scaling Limits
**Uninstall progress bar truthfulness:**
- Current capacity: Uninstall now has timeouts (fixed 2026-07-26) but progress-bar still reports fake stages (full-red full-opacity).
- Limit: Long uninstalls (>30s) show no real progress; bar claims "uninstalling" for the full duration.
- Scaling path: Backend must emit real progress events (% complete, stage name); UI must poll + display truthfully; integrate into all 5 gate iterations (not just 1 throw-away app).
**Federation node list deduplication on disk bloat:**
- Current capacity: `federation/storage.rs:dedup_nodes_by_onion()` reads entire nodes.json into memory each time a node is added/synced. At N federated peers, O(N) memory + O(N²) comparisons per operation.
- Limit: No hard limit measured; scales fine up to hundreds of peers. Beyond 1000+ peers, memory/time may become visible.
- Scaling path: Switch to a disk-backed database (e.g., rocksdb) for federation state if peer count grows; or implement incremental dedup on disk writes (preserve dedup state, only recompute on load).
**Lifecycle gate iteration count:**
- Current capacity: `ARCHY_ITERATIONS=5` runs 5 full cycles (stop/start/restart/survive per app). Entire run takes ~812 hours on .228.
- Limit: Cannot easily scale to 10+ iterations without timeout risks; per-app timeout tuning is manual.
- Scaling path: Add per-app timeout tuning (manifest field); parallelize per-app tests where safe (currently serial to avoid contention).
## Dependencies at Risk
**Reticulum transport daemon process group:**
- Risk: Pre-fix (before `be50c886`), process group wasn't cleaned up on drop. Fork-bombs or dangling processes possible under error conditions.
- Impact: Stale reticulum processes accumulating over time; resource leaks on node.
- Migration plan: Code fix already deployed (commit `7a7fec21`); no active risk. Monitor fleet for stale python processes post-deployment.
**Podman socket mount security model:**
- Risk: Apps mounting `/run/podman/podman.sock` get full container-management access. Not restricted by the security policy (AppArmor profiles not applied).
- Files: `core/archipelago/src/container/prod_orchestrator.rs:135-137` (detection), manifests for apps with podman mounts (e.g., portainer)
- Impact: A compromised app with podman socket access can start/stop/delete any container on the node.
- Recommendation: Restrict podman socket mounts to admin-only apps (portainer, docker-api tools); document risk; consider socket filtering layer (selinux context, etc.) once AppArmor is wired.
**Bitcoin version multi-version branch not fleet-wide:**
- Risk: Branch `bitcoin-version-bulletproof` (base `095a76cd`) carries multi-version support but hasn't been deployed fleet-wide yet. .228 carries it; others still run single version.
- Impact: Users on single-version nodes can't switch versions; version mismatch across fleet breaks federation.
- Migration plan: Coordinated OTA + catalog publish + `:latest` repoint sequencing per `docs/bitcoin-version-bulletproof-rollout.md`. Awaiting user decision on timing.
## Missing Critical Features
**Developer tooling CLI suite:**
- Problem: Third-party developers need `archy app validate/render/local-install/lifecycle-test` tooling before external registry launches.
- Blocks: External marketplace (workstream C); external developer onboarding.
- Status: Not yet built; documented in APP-PACKAGING-MIGRATION-PLAN.md step 5.
**Manifest-distributed registry flip:**
- Problem: Manifests still travel via OTA disk rsync. The signed catalog currently distributes only image overrides, not full manifests. Workstream B phases 1+2 done; not yet fleet-deployed.
- Blocks: Cannot confidently add/bump apps without re-signing the catalog.
- Status: Code ready; flip awaits authorization + timing call from user.
**Phase-3 Quadlet default-flip:**
- Problem: Orchestrator still uses legacy cgroup-based container management; Phase-3 `use_quadlet_backends` switch exists but is opt-in only.
- Blocks: Resolves container thrashing; unlocks independent app restarts; unblocks lifecycle perfection (workstream F).
- Status: Code validated on .228/.198 (commit pending); ready to flip when multinode gate passes.
## Test Coverage Gaps
**~30 apps with zero app-specific assertions:**
- What's not tested: Apps like grafana, jellyfin, vaultwarden, penpot, nextcloud, photoprism, uptime-kuma, homeassistant, etc. have no app-specific health checks beyond "container running."
- Files: `tests/lifecycle/bats/all-apps-matrix.bats`, `tests/lifecycle/bats/all-apps-lifecycle.bats` (generic baseline coverage)
- Risk: App-specific bugs (API down, data corruption, dependency failure) go unnoticed until user encounters them.
- Priority: Medium — baseline coverage is a real safety net; app-specific assertions are a "nice to harden" backlog item, not a gate blocker.
- Approach: Add per-app health RPC endpoints or HTTP probes; wire into the gate as opt-in per-app test suites.
**Progress UI assertions incomplete:**
- What's not tested: Install + uninstall must report monotonic, truthful progress. No stage/percentage assertions in the gate.
- Files: `neode-ui/src/components/AppCard.vue`, `core/archipelago/src/api/rpc/package/install.rs` (backend progress events)
- Risk: Silent hangs or fake progress bars are invisible to the gate.
- Priority: High — immich/grafana uninstall was stuck full-red (fixed); progress truthfulness is part of definition of done for workstream F.
- Approach: Backend must emit real progress events; UI must display & test them; integrate into canonical gate (currently opt-in).
**All-apps matrix in cascade gate:**
- What's not tested: `ARCHY_GATE_CASCADE=1` runs ONE throwaway app's uninstall/reinstall. Must extend to multi-container stacks (immich, btcpay, mempool) and all ~40 installed apps.
- Files: `tests/lifecycle/bats/cascade-uninstall.bats` (single-app variant)
- Risk: Multi-container app uninstall bugs (e.g., orphan postgres container) go undetected.
- Priority: High — part of workstream F definition of done.
- Approach: Parametrize cascade test over all manifest IDs; run 5 cascades total (not 5 per app to save time); gate-pass requires zero ghost containers post-uninstall.
---
*Analysis based on codebase state 2026-07-29. Issues tracked in `docs/UNIFIED-TASK-TRACKER.md` (day-to-day) and `docs/PRODUCTION-MASTER-PLAN.md` (historical narrative).*