|
| 1 | +# Phase 2-Globals Cheat Sheet (2-minute read) |
| 2 | + |
| 3 | +> Companion to `PHASE-2-GLOBALS-CLEANUP.md`. This is what you need to read to sign off. |
| 4 | +
|
| 5 | +## What is it (in one sentence) |
| 6 | + |
| 7 | +Clean up the 4 ambient `(global as any).X` patterns in pc2-node — replace them with the established `req.app.locals.X` pattern (where audit-correct) and document the one exception that should stay (a defensive fallback in a critical code path). **Side benefit**: this fixes one real latent bug. |
| 8 | + |
| 9 | +## The bug we accidentally found |
| 10 | + |
| 11 | +While investigating `(global as any).pc2Config`, the survey discovered that **`(global as any).db` is never set anywhere in the codebase**. Two route files (`api/resources.ts` and `api/supernode.ts`) have a `getDb()` helper that returns this never-set global. The helper always returns `undefined`. Every `db?.getSetting('storage_limit')` (and similar for max_concurrent_wasm / max_memory_mb / wasm_timeout_ms) silently falls back to config.json defaults. |
| 12 | + |
| 13 | +In plain English: **users who set their storage limit via the API see the write succeed, but the new value is ignored on every subsequent read.** The "Database settings override config file" comment in the code is currently false. |
| 14 | + |
| 15 | +Phase 2-Globals fixes this as a side effect of the cleanup. |
| 16 | + |
| 17 | +## What does NOT change |
| 18 | + |
| 19 | +- **Zero UI/UX change for the 99% case** — users who never touched their resource settings see exactly the same behavior as before (defaults applied). |
| 20 | +- **No API contract change**. All endpoints return the same shape. |
| 21 | +- **No config change**. No env vars, no flags. |
| 22 | + |
| 23 | +## What DOES change |
| 24 | + |
| 25 | +1. **For users who set resource limits via the API**: their settings start being honored (bug fix). This includes `storage_limit`, `max_concurrent_wasm`, `max_memory_mb`, `wasm_timeout_ms`. |
| 26 | +2. **One less attack surface**: removing `pc2Config` mutable global also removes a mutable-shared-state risk pattern that capsule-aware code in the Runtime cannot have. |
| 27 | +3. **Cleaner codebase for the next audit**: every `(global as any).X` pattern in pc2-node is either deleted or has a comment explaining why it's intentional. |
| 28 | + |
| 29 | +## The 4 globals at a glance |
| 30 | + |
| 31 | +| # | Global | What we do | Why | |
| 32 | +|---|---|---|---| |
| 33 | +| 1 | `pc2Config` | **Delete entirely**. Readers go to db (the real source of truth) directly. | Vestigial cache; only 1 of 4 mutable settings was ever cached; redundant with `db.setSetting()` | |
| 34 | +| 2 | `global.db` | **Delete the broken `getDb()` helpers**. Replace with `req.app.locals.db` lookups. | Latent bug — was never set; fixes db-persisted setting reads | |
| 35 | +| 3 | `__filesystem` | **Keep as-is + add explanatory comment**. | Deliberate defensive fallback for the Drivers tool-execution critical path. Intentional, not ambient authority. | |
| 36 | +| 4 | `ipfsStorage` | **Delete the helper**, replace reads with `req.app.locals.ipfs`. Keep the bootstrap single-write at startup. | Same pattern as 2-C — single-write at bootstrap is audit-permitted; the consumer helpers should use Express context. | |
| 37 | + |
| 38 | +## Files involved |
| 39 | + |
| 40 | +**Will be modified (5 files)**: |
| 41 | +- `pc2-node/src/api/storage.ts` — delete pc2Config write block |
| 42 | +- `pc2-node/src/api/resources.ts` — delete `getDb()` + `getConfig()` helpers; thread `db` parameter; replace 4 call sites |
| 43 | +- `pc2-node/src/api/info.ts` — drop pc2Config fallback (1 line) |
| 44 | +- `pc2-node/src/services/wasm/WASMRuntime.ts` — drop pc2Config read (1 line; behavior unchanged because compute settings never wrote to pc2Config anyway) |
| 45 | +- `pc2-node/src/api/supernode.ts` — delete `getDb()` + `getIpfs()` helpers; replace 4 call sites |
| 46 | + |
| 47 | +**Comment-only additions (2 files)**: |
| 48 | +- `pc2-node/src/server.ts:141` — explain `__filesystem` defensive fallback intent |
| 49 | +- `pc2-node/src/api/other.ts:882` — same |
| 50 | + |
| 51 | +## Risk profile |
| 52 | + |
| 53 | +| Concern | Phase 2-C | Phase 2-D | Phase 2-Globals (this ticket) | |
| 54 | +|---|---|---|---| |
| 55 | +| Compiled JS change | Yes (route handlers now `req.app.locals.X`) | None (byte-identical) | Yes (route handlers + WASMRuntime literal change) | |
| 56 | +| Behavior change | None (verified `version: "1.2.7.14"` round-trip) | None (byte-identical) | **Yes — deliberate**: db-persisted settings start being honored | |
| 57 | +| New code introduced | Constructor injection at 1 class | None (keyword change only) | Helper deletion + parameter threading in 1 function | |
| 58 | +| User-visible effect | None | None | **One bug fix** — for users who used the API to set limits, their settings now work | |
| 59 | + |
| 60 | +This is the first Phase 2 ticket where the source-code change has **deliberate user-visible behavioral implications**. Per ticket §"Behavioral change (deliberate)", this is a **fix**, not a regression — but it warrants careful release-notes communication. |
| 61 | + |
| 62 | +## The 4 sign-off questions |
| 63 | + |
| 64 | +1. **Approve the deliberate `global.db` bug fix?** Recommendation: yes — db-persisted settings being silently ignored is incorrect behavior; fixing it is a correctness improvement. Document in release notes as "Bug fix: resource limit settings (storage_limit, max_concurrent_wasm, max_memory_mb, wasm_timeout_ms) now correctly applied". |
| 65 | +2. **Defer the WASMRuntime config-injection refactor** (would make compute settings injectable from db at startup, opening path to runtime-mutable compute limits) — defer to a follow-up ticket, or fold in here? Recommendation: defer — this ticket is already ~3 hours. |
| 66 | +3. **Bundle all 4 globals into one PR** or split into 4 PRs? Recommendation: bundle (matches Phase 2-C precedent of one PR per Phase letter). |
| 67 | +4. **Execute now on feature branch, ship after Mac soak** — same model as 2-B/2-C/2-D? Recommendation: yes, but include a pre-merge step to check existing user dbs for non-default resource settings (so we know what behavioral changes a real user might see). |
| 68 | + |
| 69 | +## Expected outcome of execution |
| 70 | + |
| 71 | +- 5 files modified (source), 2 files comment-only updates, ~12 sites total changed. |
| 72 | +- `tsc --noEmit` clean, `build:backend` clean, `test:unit` 7/7, ReadLints zero errors. |
| 73 | +- Compiled JS changes per file (NOT byte-identical, unlike 2-D) — the changes are real behavioral fixes. |
| 74 | +- Dev server `/api/health`, `/api/info`, `/api/storage/usage`, `/api/resources` all return correct values. |
| 75 | +- **New round-trip test**: POST `/api/storage/limit` with a non-default value → GET `/api/info` → response reflects the user's setting (this currently fails; should pass after). |
| 76 | +- CI green on 4-platform matrix in ~10 min. |
| 77 | +- Audit's ambient-global Pattern #2 work is now functionally complete. |
| 78 | + |
| 79 | +--- |
| 80 | + |
| 81 | +If you say "ok lets do it", I'll execute, validate, commit, push, and post the CI-green confirmation. Same workflow as 2-B, 2-C, 2-D — plus an additional pre-execution step to grep the production user-db schema for any non-default resource settings (to know what behavioral fixes a real user would see). |
0 commit comments