amadan
geoip spec rev 6 (Astra round 5) and plan 1 to match
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+69
−14
docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md+10 −4docs/superpowers/specs/2026-10-06-platform-geoip-design.md+39 −10docs/superpowers/specs/CODEX-FINDINGS-GEOIP-R5.md+20 −0
CI cancelled — superseded by a newer push — run details
diff --git a/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md b/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md| index d25916679..be83b0d97 100644 |
| --- a/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md |
| +++ b/docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md |
| @@ -19,7 +19,7 @@ |
| **Tech Stack:** Go, `github.com/oschwald/maxminddb-golang` v1.13.1 (already in `go.mod`), `net/http/httputil.ReverseProxy`, AWS SDK v2 S3, OpenTofu (`infra/modules/deployment-buckets`). |
| -**Spec:** `docs/superpowers/specs/2026-10-06-platform-geoip-design.md` (rev 5). Read §0 rulings first. Measured facts: `docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md`. |
| +**Spec:** `docs/superpowers/specs/2026-10-06-platform-geoip-design.md` (rev 6). Read §0 rulings first. Measured facts: `docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md`. |
| ## Global Constraints |
| @@ -3679,7 +3679,7 @@ func TestBoundedAddFoldsIntoOther(t *testing.T) { |
| Replace `newTestMeter` and `onlyDelta` with the helpers `meter_test.go` already uses to build a `Meter` and drain one delta (`grep -n "func new.*Meter\|Drain()" internal/analytics/*_test.go`). |
| -In `internal/edge/tunnel_geo_e2e_test.go`, give the hub and the remote edge an analytics meter each (`EnableAnalytics` with the app switched on through `SetAnalyticsOn`), send one fresh request and one that the hub answers from its cache, then drain both meters. Across the two meters, pageviews, `Countries["GB"]` and `GeoSources` must each total exactly 2: one per served response, never 3 or 4. The same must hold over a plaintext tunnel, where pageviews total 2 and countries 0. |
| +In `internal/edge/tunnel_geo_e2e_test.go`, give the hub and the remote edge an analytics meter each (`EnableAnalytics` with the app switched on through `SetAnalyticsOn`), send one fresh request and one that the hub answers from its cache, then drain both meters. The remote meter must record **zero** observations. The hub's meter must record pageviews, `Countries["GB"]` and `GeoSources` of exactly 2 each, one per served response (spec §8.4, §9). The same must hold over a plaintext tunnel, where pageviews total 2 and countries 0. |
| In `internal/edge/analytics_test.go`, extend `TestEdgeObservesOnlyWhenSwitchedOn` (or add a sibling test). With the City fixture in `e.geo`, a pageview from 81.2.69.142 records `Obs.Country == "GB"`, `GeoState == "hit"` and `GeoSource == "GeoIP2-City"`. Also fill in the Task 10 marker in `tunnel_geo_e2e_test.go`. |
| @@ -3875,7 +3875,8 @@ git commit -m "analytics: count pageviews by country with per-day source provena |
| **The verdict** (spec §5.5, codex R4 on R1-16): |
| - **Expected boxes:** the box names under `control/edges/` (list the prefix; strip `.json`). |
| -- **Heartbeats:** `status.LoadBoxesComplete` on the platform prefix, plus each fleet prefix read the way `systemBoxes(ctx, true)` reads them. |
| +- **Expected fleet boxes:** for each fleet from the registry (`reg.ListFleets()`), its active boxes from `reg.ListFleetBoxes(name)`. Open the registry the way `systemBoxes` does (`s.openPassRegistry()`). Fleet boxes publish no edges record (spec §5.5, codex R5 on R1-16). |
| +- **Heartbeats:** `status.LoadBoxesComplete` on the platform prefix and on every `release.NewPrefixStore(s.cfg.Releases, "fleets/"+name+"/")`. Never `LoadBoxes` or `systemBoxes`, which skip unreadable records. |
| - **Listed boxes:** every expected box, plus any heartbeat box whose `Updated` is within 7 days. |
| - An expected box with no heartbeat blocks, as `"<label>: no heartbeat"`. |
| - An incomplete read blocks, as `"heartbeats: unreadable"`. |
| @@ -3921,7 +3922,12 @@ Expected: FAIL. |
| **Publish as upload then job** (spec §4.2, codex R4-2). Split `geoipPublisher.Publish` into two methods. The tests in Task 6 keep calling `Publish`, which becomes `Stage` followed by `Commit`. |
| -- `Stage(ctx, src) (staged, error)` copies the upload to the temp dir under `copyCapped`. Before copying it takes the mutex with `TryLock`, and returns `errGeoipBusy` if it can't. On success the mutex stays held and passes to the job, which releases it. |
| +- `Stage(ctx, src) (staged, error)` copies the upload to the temp dir under `copyCapped`. Before reading **any** of the body it takes the mutex with `TryLock`, and returns `errGeoipBusy` if it can't. That reservation is spec §4.2's one slot (codex R5-1). On success the mutex stays held and passes to the job, which owns the temp dir from then on and releases the mutex after removing it. On failure, `Stage` removes the dir and unlocks. |
| +- Add `TestSecondUploadIsRefusedBeforeItsBodyIsRead` in `apisystemgeoip_test.go`: |
| + - start PUT #1 with an `io.Pipe` body that writes 1 KiB and then blocks; |
| + - wait until the handler holds the mutex (poll `p.mu.TryLock()`, which fails while it's held); |
| + - send PUT #2 with a body that records whether `Read` was ever called; |
| + - PUT #2 must get 409, and its body's `Read` must never be called. |
| - `Commit(ctx, staged, opts) (geoip.Pointer, error)` runs §4.1 steps 3 to 6, then removes the temp dir and unlocks. |
| The handler: |
diff --git a/docs/superpowers/specs/2026-10-06-platform-geoip-design.md b/docs/superpowers/specs/2026-10-06-platform-geoip-design.md| index b87d042e2..a2b7933b4 100644 |
| --- a/docs/superpowers/specs/2026-10-06-platform-geoip-design.md |
| +++ b/docs/superpowers/specs/2026-10-06-platform-geoip-design.md |
| @@ -1,7 +1,7 @@ |
| # Platform GeoIP for apps: headers, a mirrored database, a lookup endpoint |
| **Date:** 2026-10-06 |
| -**Revision:** 5 |
| +**Revision:** 6 |
| **Discussion:** amadan carlos/platform #241 (quoted in Appendix A) |
| **Branch:** `geoip-for-apps` |
| @@ -44,6 +44,11 @@ |
| - Publishing becomes an upload followed by a job. |
| - A tunnelled request is counted once, by the hub. |
| - The rollout verdict reconciles against the edges records. |
| +- Rev 6 answers Astra's round 5 (`CODEX-FINDINGS-GEOIP-R5.md`): 1 new P2 |
| + and 2 partly resolved findings, all accepted, marked "(codex R5-N)". |
| + - The upload slot is reserved before any body is read. |
| + - Fleet boxes are reconciled through the registry. |
| + - The tunnel analytics test is corrected. |
| ## 0. Rulings and decisions |
| @@ -302,6 +307,18 @@ Under `carlos system`, through the console API: |
| `http.NewResponseController`, the way the ship door does |
| (`handleAPIShip`, survey), and wraps the body in `MaxBytesReader` at |
| 1 GiB. |
| + - **One slot, reserved first** (codex R5-1). The handler takes the |
| + publish mutex (`TryLock`) **before reading any of the body**. If it |
| + is held, by another upload still streaming or by a job, the answer |
| + is 409 straight away. So at most one upload is ever staged on the |
| + console's disk. |
| + - The mutex and the temporary directory pass from the handler to |
| + the job. The job releases the mutex only after removing the |
| + directory. |
| + - If the upload fails or is cut off before the job starts, the |
| + handler removes the directory and releases the mutex itself. |
| + - A test stalls a first upload's body and checks that a second |
| + upload gets 409 without any of its body being read. |
| - The handler only stages the upload: it streams it to the publish |
| path's temporary file and hashes it. |
| - It then answers **202 with a job id** straight away. A request that |
| @@ -492,14 +509,17 @@ fleet boxes write under `fleets/<fleet>/status/boxes/`: |
| `carlos system geoip status` reconciles two lists (codex R4 on R1-16). |
| **What it reads:** |
| -- **Expected boxes:** every box that publishes an edges record |
| - (`control/edges/<box>.json`, what the home map reads). These boxes serve |
| - traffic. |
| -- **Heartbeats:** |
| - - the platform prefix, read with `status.LoadBoxesComplete`, which says |
| - when any record couldn't be read, unlike `LoadBoxes`, which skips |
| - unreadable records silently; |
| - - every fleet prefix, the way `systemBoxes` reads them, in strict mode. |
| +- **Expected boxes:** |
| + - every platform box that publishes an edges record |
| + (`control/edges/<box>.json`, what the home map reads); |
| + - every active fleet box the registry lists (`ListFleetBoxes`). Fleet |
| + boxes don't publish edges records (`internal/host/edgesfile.go`), so |
| + they are reconciled against the registry instead (codex R5 on R1-16). |
| +- **Heartbeats:** every prefix (the platform's and each fleet's |
| + `fleets/<name>/status/boxes/`) is read with `status.LoadBoxesComplete`, |
| + which says when any record couldn't be read. Neither `LoadBoxes` nor |
| + `systemBoxes`'s strict mode is used, because both skip unreadable |
| + records silently. |
| **Which boxes are listed:** |
| - Every expected box is listed whatever its age. |
| @@ -715,7 +735,8 @@ under the same values `setGeo` writes, because both come from `geoResult`. |
| nomination, including a cache replay; |
| - a forged header on a tunnel box's public listener; |
| - a request through a real `tunnelclient` connection that adopts the |
| - hub's headers and counts the country in the remote box's analytics; |
| + hub's headers. The remote box makes **zero** analytics observations, |
| + and the hub exactly one (codex R5 on R4-1, §9); |
| - a door call to an app whose home is another box, which reaches the app |
| with no geo headers; |
| - a hop signed with a key the verifier doesn't have yet (the rotation |
| @@ -1194,6 +1215,14 @@ Round 4 (`CODEX-FINDINGS-GEOIP-R4.md`): every finding accepted. |
| | 1 | P2 | Accepted. The remote edge skips analytics for tunnelled requests, via a trust-free `viaTunnelKey`; this fixes the existing pageview double count too (§9) | |
| | 2 | P2 | Accepted. Upload then a job on the existing system job registry; 202, then the CLI polls (§4.2) | |
| +Round 5 (`CODEX-FINDINGS-GEOIP-R5.md`): every finding accepted. |
| + |
| +| # | Sev | Disposition | |
| +|---|---|---| |
| +| R1-16 (partly) | | Fleet boxes reconciled via `ListFleetBoxes`; `LoadBoxesComplete` on every prefix (§5.5) | |
| +| R4-1 (partly) | | The §8.4 test now asserts zero remote observations and one at the hub | |
| +| 1 | P2 | Accepted. The publish slot is reserved before the body is read, and handed to the job (§4.2) | |
| + |
| ## Appendix A. The request (#241, verbatim) |
| > **Expose the platform's GeoIP lookup to apps (headers and/or lookup endpoint)** |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-GEOIP-R5.md b/docs/superpowers/specs/CODEX-FINDINGS-GEOIP-R5.md| new file mode 100644 |
| index 000000000..a2c4ac4fe |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-GEOIP-R5.md |
| @@ -0,0 +1,20 @@ |
| +Round 5 review of `feef9a5c`. |
| + |
| +| Previous finding | Disposition | Evidence from revision 5 | |
| +|---|---|---| |
| +| R3-1. Expiry/install races | Resolved | §5.3 retains expiry checks before download and installation, with `genMu` serializing installation, expiry and cleanup. | |
| +| R3-2. Unprotected console `Locate` | Resolved | §5.4 retains locked `Swappable.Locate` and puts the wrapper in `Config.GeoIP`. | |
| +| R3-3. Coordinate-only results | Resolved | §8.1a accepts coordinate-only hits; §9 counts their country as `_unknown`. | |
| +| R3-4. Heartbeat freshness | Resolved | §5.5 retains `status.BoxStale` and the jittered-heartbeat test. | |
| +| R1-4, carried forward: orphan destruction | Resolved | §3 retains startup and daily orphan sweeps, followed by §15’s noncurrent-version expiration. | |
| +| R1-16, carried forward: rollout verification | Partly resolved | §5.5 adds expected-box reconciliation through `control/edges/`, but fleet boxes explicitly skip that publisher: [edgesfile.go:698](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/host/edgesfile.go:698). Missing fleet heartbeats remain invisible, and those older than seven days are still discarded. Also, `systemBoxes`’s strict mode still uses `LoadBoxes`, which silently skips individual unreadable records: [apisystemupdate.go:258](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/console/apisystemupdate.go:258). Reconcile active registered fleet boxes through `ListFleetBoxes`, and use `LoadBoxesComplete` for every heartbeat prefix. | |
| +| R4-1. Duplicate tunnel analytics | Partly resolved | §9 correctly suppresses the remote observer through `viaTunnelKey`, but §8.4 still requires a test that counts the country in the remote box’s analytics. Change that assertion to zero remote observations and exactly one aggregate observation at the hub. | |
| +| R4-2. Publication deadline across backhaul | Resolved | §4.2 stages the upload, returns 202, and moves verification and publication into a polled job, with a backhaul timeout regression test. | |
| + |
| +1. **P2 — §§4.1–4.2: the upload-and-job split leaves staging concurrency unspecified.** The revised protocol places the job under the publication mutex and refuses another upload while a *job* holds it. It does not reserve the slot while the first request is still uploading. Multiple slow uploads can therefore start before any job exists, each staging up to 1 GiB on the console’s disk. Serializing the subsequent jobs does not bound that disk consumption. |
| + |
| + **Evidence:** the staging and job-lock rules are at [design.md:305](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/docs/superpowers/specs/2026-10-06-platform-geoip-design.md:305). The existing job registry reserves its slot only when `startSystemJob` runs: [apisystemupdate.go:364](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/console/apisystemupdate.go:364). Reusing the registry after staging supplies no upload admission control. |
| + |
| + **Fix:** reserve one publication slot before reading any upload body, retain it through the handler-to-job handoff, and release it after job cleanup. Explicitly transfer ownership of the temporary file to the job. Test two concurrent uploads with the first body stalled: the second must receive 409 before its body is read. |
| + |
| +Verdict: revise. |
| \ No newline at end of file |