geoip spec rev 7 (Astra round 6: D2 transport-independent tunnel mark, staging cleanup, explicit retirement) and plan 1 to match
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
3 files changed,
+119
−28
docs/superpowers/plans/2026-10-06-platform-geoip-plan1.md+26 −8docs/superpowers/specs/2026-10-06-platform-geoip-design.md+66 −20docs/superpowers/specs/CODEX-FINDINGS-GEOIP-R6.md+27 −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 be83b0d97..98c750bd8 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 6). 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 7). Read §0 rulings first. Measured facts: `docs/superpowers/plans/2026-10-06-platform-geoip-FACTS.md`. |
| ## Global Constraints |
| @@ -3404,7 +3404,7 @@ git commit -m "edge: capture/decide/expose geo headers; setGeo on every proxy; d |
| --- |
| -### Task 10: The TLS-only tunnel mark |
| +### Task 10: The tunnel mark (spec D2: every tunnelled request, whatever the scheme) |
| **Files:** |
| - Modify: `internal/tunnelclient/tunnelclient.go` (`Config` :66; `Loop` :87; `serveUntilDead` :158; `Dial` :191; `Serve` :265) |
| @@ -3417,10 +3417,11 @@ git commit -m "edge: capture/decide/expose geo headers; setGeo on every proxy; d |
| // package tunnelclient |
| func FromTunnel(ctx context.Context) bool |
| // Config gains: TLSConfig *tls.Config // nil = system roots (production); tests pass the test server's pool |
| - func Mark(h http.Handler) http.Handler // wraps h so every request carries the TLS trust mark |
| - func ViaTunnel(ctx context.Context) bool // true for EVERY request served over a tunnel, TLS or not (spec §9, codex R4-1) |
| + func Mark(h http.Handler) http.Handler // wraps h so every request carries the mark |
| ``` |
| - `Loop` always puts the via-tunnel mark on requests. It adds the trust mark (`Mark`) only when `cfg.URL`'s scheme is `https`. |
| + `Loop` serves `Mark(cfg.Handler)` whatever `cfg.URL`'s scheme (spec D2). The mark means the hub computed this request's geo answer. The remote adopts that answer and does not observe the request for analytics. |
| + |
| + **Ignore** every mention below of `ViaTunnel`, `viaTunnelKey` or `viaHandler`, and of "TLS only" or "plaintext gets no geo". Rev 7's D2 removed them. There is one mark, `FromTunnel`, and `servedHandler(cfg, h)` returns `Mark(h)` unconditionally. `TestServeHandlerMarksTrustOnlyOverTLSButViaAlways` becomes `TestServeHandlerAlwaysMarks`: plain and TLS both see `FromTunnel == true`. The `Config.TLSConfig` seam is still added, so the e2e can run over real TLS. `observeAnalytics` (Task 11) checks `tunnelclient.FromTunnel`. |
| - [ ] **Step 1: Write the failing tests.** `internal/tunnelclient/mark_test.go`: |
| @@ -3473,7 +3474,7 @@ Assert: |
| - a request to the hub from `RemoteAddr` 81.2.69.142 (City fixture loaded in the hub's `e.geo`) reaches the remote app with `X-Carlos-Geo-Country: GB` and no `X-Carlos-Hop-Geo`; |
| - the remote edge's `e.geo` is empty and was never consulted; |
| - the remote edge's analytics meter records `Country: "GB"` (this assertion lands in Task 11; leave a `// Task 11` marker here); |
| -- the same scene over plain `http://` delivers **no** geo headers. |
| +- the same scene over plain `http://` delivers the **same** geo headers (spec D2). Cover both schemes in one table-driven test. |
| Factor `startFakeRemoteHost` (`tunnel_e2e_test.go:190`) so it takes a `tunnelclient.Config` mutator, and don't copy it. |
| @@ -3679,7 +3680,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. 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/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 (D2): the remote records zero, and the hub records 2 pageviews and 2 for `Countries["GB"]`. |
| 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`. |
| @@ -3763,7 +3764,7 @@ func mergeCounts(dst, src map[string]int64, max int) map[string]int64 { |
| ```go |
| // One observation per served response (geoip spec §9, codex R4-1): the |
| // hub that forwarded this over the tunnel already observed it. |
| - if tunnelclient.ViaTunnel(r.Context()) { |
| + if tunnelclient.FromTunnel(r.Context()) { |
| return |
| } |
| ``` |
| @@ -3996,6 +3997,23 @@ The DELETE calls `s.geoip.Off`, then answers `feed_set: os.Getenv("CONSOLE_GEOIP |
| Update the comment block above them to say the geoip doors use `apiSystemRoller`. |
| +**Staging directory** (spec §4.2, codex R6-2): `geoipTmpRoot(cfg)` is `filepath.Join(filepath.Dir(<CONSOLE_DB path>), "geoip-staging")`. `newGeoipPublisher` does two things at construction, before the server admits any request: |
| +- `os.RemoveAll(tmpRoot)`; |
| +- `os.MkdirAll(tmpRoot, 0o700)`. |
| + |
| +Add `TestPublisherStartupClearsAbandonedStaging` to `geoippublish_test.go`: |
| +- create `tmpRoot/geoip-publish-123/db.mmdb` by hand, standing in for a crash mid-stage; |
| +- construct a publisher on that root, and check the directory is now empty; |
| +- check that a `Publish` then succeeds. |
| + |
| +**Retirement instruction** (spec §5.5, codex R6 on R1-16): an expected box whose heartbeat is stale gets a `blocking` entry of the form `"<label>: no heartbeat since <RFC3339 date>. if this box is retired, delete its edges record (control/edges/<label>.json)"`. That string is copy, so add it to Task 13's list. Also file the discussion: |
| + |
| +```bash |
| +amadan discuss new carlos/platform -branch geoip-for-apps -title "A carlos system verb to retire a box (delete its edges record)" -body - <<'BODY' |
| +Nothing deletes a retired box's control/edges/<label>.json (instancesplacement.go says so). The geoip rollout verdict (geoip spec §5.5) blocks on an expected box with a stale heartbeat and tells the operator to delete the record by hand. A first-class verb would also stop the home map backhauling to a dead box. Filed rather than worked around. |
| +BODY |
| +``` |
| + |
| **Server wiring:** |
| - Where `Server` is constructed and `cfg.Releases` is known, set: |
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 a2b7933b4..264d59e6b 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:** 6 |
| +**Revision:** 7 |
| **Discussion:** amadan carlos/platform #241 (quoted in Appendix A) |
| **Branch:** `geoip-for-apps` |
| @@ -24,7 +24,7 @@ |
| - Rev 3 adds an internal trust header, `X-Carlos-Hop-Geo`. It carries the |
| ingress edge's state (hit, miss, off) across a hop or the tunnel, and |
| no app ever sees it. |
| - - It withholds geo on a rejected hop and on a plaintext tunnel. |
| + - It withholds geo on a rejected hop (and, until rev 7, on a plaintext tunnel; see D2). |
| - It adds a bucket lifecycle rule for GeoLite2's destroy duty. |
| - Rev 4 answers Astra's round 3 (`CODEX-FINDINGS-GEOIP-R3.md`). That round |
| resolved all 9 of round 2's findings, left 2 earlier findings partly |
| @@ -49,6 +49,11 @@ |
| - The upload slot is reserved before any body is read. |
| - Fleet boxes are reconciled through the registry. |
| - The tunnel analytics test is corrected. |
| +- Rev 7 answers Astra's round 6 (`CODEX-FINDINGS-GEOIP-R6.md`), marked |
| + "(codex R6-N)". |
| + - It takes **D2**, which reverses round 2's TLS-only tunnel trust. |
| + - Uploads stage in a directory the console clears at startup. |
| + - A box is retired explicitly, by deleting its edges record. |
| ## 0. Rulings and decisions |
| @@ -100,6 +105,28 @@ reverse. |
| - **DB-IP Lite has no such duty** (CC BY 4.0) and never expires. It |
| only alerts as stale. |
| +- **D2 (codex R6-1, reversing the R2-2 answer). Every request served over |
| + the tunnel carries the hub's answer, TLS or not.** |
| + - Round 2 withheld geo trust from plaintext tunnels, because nothing |
| + authenticates the hub to the box there. |
| + - That protects nothing. An on-path attacker who can pose as the hub on |
| + a plaintext tunnel can already inject any request at all: method, |
| + path, body, cookies, `X-Forwarded-For`. Geo headers add nothing to |
| + what such an attacker controls. |
| + - Withholding them did cost something. The hub's cache stored variants |
| + under a country the app never received, and those variants were |
| + replayed after the box reconnected over TLS. The hub's analytics |
| + counted a country that §9's plaintext rule said shouldn't exist. |
| + - So one mark, set on every request `tunnelclient` serves, means "the |
| + hub computed this request's answer". The remote adopts that answer |
| + and doesn't observe the request for analytics. |
| + - Plaintext tunnels remain what `tunnelclient.Dial` already calls them: |
| + "for tests". The operator docs say production tunnels use `https://` |
| + (`infra/remote-host.md` already does). |
| + - Because the hub's answer always reaches the app, the variant the hub |
| + caches always matches what the app saw, whatever the transport, on |
| + reconnect and on fleet fallback. |
| + |
| ## 1. Why, and what this is not |
| The nearest-region work gave the console a GeoIP reader (`internal/geo`, |
| @@ -321,6 +348,13 @@ Under `carlos system`, through the console API: |
| 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. |
| + - **The staging directory** is `<console data dir>/geoip-staging/`, |
| + next to `CONSOLE_DB`, and used for nothing else. The console clears |
| + it at startup, before it admits any upload or starts the feed, so a |
| + crash during staging or during a job never leaves its 1 GiB behind |
| + for good (codex R6-2). A test kills the publisher mid-stage and |
| + mid-job, then checks that a fresh publisher on the same directory |
| + starts empty and accepts a retry. |
| - It then answers **202 with a job id** straight away. A request that |
| came through another edge's backhaul has 30 seconds after its body |
| to see response headers (`backhaulHeaderTimeout`), and verifying |
| @@ -541,6 +575,19 @@ It reports: |
| it strips forged ones. |
| - **`mode: mirror`:** shows the box's sha against the pointer's. |
| +**Retiring a box** (codex R6 on R1-16). Nothing deletes a retired box's |
| +edges record today (`internal/console/instancesplacement.go`, "nothing |
| +deletes a retired box's file"). A stale heartbeat is never taken as |
| +retirement. |
| +- An expected box whose heartbeat is stale blocks the verdict, with the |
| + instruction: "no heartbeat since <date>. if this box is retired, delete |
| + its edges record (control/edges/<label>.json)". |
| +- Deleting that record is the explicit retirement. It also stops the home |
| + map backhauling traffic to a dead box. |
| +- A `carlos system` verb to retire a box belongs to the platform, not to |
| + GeoIP. Plan 1 files it as a discussion rather than working around its |
| + absence. |
| + |
| It ends with a one-line verdict. **"Ready"** means every listed box is |
| fresh, readable and GeoIP-aware, and every mirroring box has loaded the |
| current sha. Anything else names the boxes in the way (codex R2 on R1-16). |
| @@ -642,8 +689,8 @@ X-Carlos-Hop-Geo: off |
| ``` |
| - `hit` and `miss` mean a reader was loaded and answered. `off` means no |
| - lookup was attempted: no database, a door request, a rejected hop, or a |
| - plaintext tunnel. |
| + lookup was attempted: no database, a door request, or a rejected |
| + hop. |
| - It is set only by `backhaulProxy`, `tunnelProxy` and `fleetProxy`, and |
| only from `geoResult`. |
| - It is stripped at capture (step 1) on every request, and adopted only |
| @@ -671,8 +718,7 @@ the validated headers it received. |
| | Door request (edge-local socket) | **`off`.** The door replaces `RemoteAddr` with the box's own public address, so a lookup would locate the box. | |
| | Sidecar through loopback | `miss`, because the address isn't locatable | |
| | Verified backhaul hop (home edge) | Adopt the sending edge's validated headers and state. No lookup. | |
| -| Request over a TLS tunnel (remote box) | Adopt the hub's validated headers and state. No lookup. | |
| -| Request over a plaintext tunnel | **`off`.** Nothing authenticates the hub (§8.4). | |
| +| Request over the tunnel (remote box), TLS or not | Adopt the hub's validated headers and state. No lookup (D2). | |
| | **Rejected hop:** it carries `X-Carlos-Hop`, but `acceptHop` didn't verify it | **`off`** (codex R2-1). The request is served as today, as an ordinary client with its forwarded headers stripped, but geo is withheld, because its `RemoteAddr` is the sending edge. A client that sends a bogus `X-Carlos-Hop` only suppresses its own geo. | |
| | Any other request | The same as a direct request | |
| @@ -708,14 +754,9 @@ under the same values `setGeo` writes, because both come from `geoResult`. |
| - **Tunnel:** a new context mark, `tunnelKey{}`. |
| - `tunnelclient` wraps the handler it gives `http2.Server.ServeConn`, so |
| every request it serves carries the mark, and nothing else sets it. |
| - - **The mark is granted only when the tunnel was dialled over TLS** (the |
| - `https` scheme, with certificate verification against the hub's name) |
| - (codex R2-2). `tunnelclient` also accepts plain `http` |
| - (`tunnelclient.go`, the dial switch). The registration token |
| - authenticates the box to the hub, not the hub to the box, so over |
| - plain HTTP an on-path peer could pose as the hub. Plaintext tunnels |
| - keep working, with geo `off`. |
| - - A test proves that a plaintext tunnel never sets the mark. |
| + - The mark is set whatever the tunnel's scheme (D2). |
| + - The same mark tells the remote edge not to observe the request for |
| + analytics (§9). |
| - Tunnel boxes do run the ordinary edge listeners on :80 and :443 |
| alongside the tunnel (`internal/host/agent.go`, `tunnelLoop`). A |
| request through those listeners doesn't carry the mark, so it is |
| @@ -764,8 +805,7 @@ show that, and only then publishes a database and the app docs. |
| - `hit` counts the country, or `"_unknown"` for a coordinate-only result |
| (codex R3-3). |
| - `miss` counts `"_unknown"`. |
| - - `off` (no database, a door call, a rejected hop, a plaintext tunnel) |
| - isn't counted in `Countries` at all, so "no GeoIP" never looks like |
| + - `off` (no database, a door call, a rejected hop) isn't counted in `Countries` at all, so "no GeoIP" never looks like |
| "visitors from nowhere". |
| - The state crosses hops and the tunnel in `X-Carlos-Hop-Geo` (§8.1a), so |
| the edge that observes the pageview counts it the way the ingress edge |
| @@ -779,10 +819,8 @@ show that, and only then publishes a database and the app docs. |
| - **Rule:** the remote edge doesn't observe a request that arrived over |
| the tunnel. The hub has already observed it, including when the hub |
| served it from its cache, where the remote never sees it. |
| - - **Mark:** `tunnelclient` puts a second context mark, `viaTunnelKey`, |
| - on every request it serves, TLS or not. Suppressing a count needs no |
| - trust, so it covers plaintext tunnels too. The TLS-only `tunnelKey` |
| - stays the geo-trust mark. |
| + - **Mark:** the tunnel mark, `tunnelKey`, which `tunnelclient` puts on |
| + every request it serves (§8.4, D2). |
| - **Scope:** usage accounting (`usageMeter`) is a separate meter, and |
| this doesn't change it. |
| - **Test:** a real hub and remote edge with both meters on, both |
| @@ -1223,6 +1261,14 @@ Round 5 (`CODEX-FINDINGS-GEOIP-R5.md`): every finding accepted. |
| | 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) | |
| +Round 6 (`CODEX-FINDINGS-GEOIP-R6.md`): every finding accepted. |
| + |
| +| # | Sev | Disposition | |
| +|---|---|---| |
| +| R1-16 (partly) | | Explicit retirement by deleting the edges record; a stale expected box blocks, with that instruction; a platform retire verb is filed as a discussion (§5.5) | |
| +| 1 | P2 | Accepted, through D2: the tunnel mark is transport-independent, so the hub's cache, the app's headers and the analytics always agree (§0, §8.2, §8.4, §9) | |
| +| 2 | P2 | Accepted. A dedicated staging directory, cleared at console startup (§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-R6.md b/docs/superpowers/specs/CODEX-FINDINGS-GEOIP-R6.md| new file mode 100644 |
| index 000000000..286e6ba77 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-GEOIP-R6.md |
| @@ -0,0 +1,27 @@ |
| +Round 6 review of `00ace13c`. |
| + |
| +| Previous finding | Disposition | Evidence from revision 6 | |
| +|---|---|---| |
| +| R3-1. Expiry/install races | Resolved | §5.3 retains expiry checks before download and installation, with `genMu` serializing selection, 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 bucket sweeps, followed by §15’s noncurrent-version expiration. | |
| +| R1-16, carried forward: rollout verification | Partly resolved | §5.5 now reconciles active fleet registrations and uses `LoadBoxesComplete` everywhere. However, every platform edges record remains expected forever, while retired boxes retain those records: [instancesplacement.go:86](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/console/instancesplacement.go:86). A retired box therefore permanently blocks “ready.” Define an explicit retirement mechanism that removes it from expected inventory; stale heartbeats alone must not establish retirement. | |
| +| R4-1. Duplicate tunnel analytics | Resolved | §8.4 now requires zero remote observations and exactly one at the hub, matching §9. | |
| +| R4-2. Publication deadline across backhaul | Resolved | §4.2 retains staging followed by a 202 response and a polled publication job. | |
| +| R5-1. Concurrent upload staging | Resolved | §4.2 reserves the mutex before reading the body, transfers mutex and directory ownership to the job, and tests refusal while the first upload is stalled. | |
| + |
| +1. **P2 — §§8.2–8.4, 9: plaintext tunnel suppression breaks the hub’s cache contract.** The hub computes geo headers, but the plaintext tunnel receiver discards them. An app returning `Vary: X-Carlos-Geo-Country` therefore produces a response without receiving a country, while the hub caches that response under, for example, `IE`. After the channel reconnects over TLS, the hub can replay it although the app would now receive `IE`. Fleet fallback between plaintext and TLS connections has the same problem. Hub-only analytics also retains the ingress country despite §9’s plaintext-`off` rule. |
| + |
| + **Evidence:** [edge.go:1657](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/edge.go:1657) matches cached variants against the hub’s request; [edge.go:1776](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/edge.go:1776) stores its `Vary` values. Connection selection happens later at [tunnel.go:393](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/tunnel.go:393), and reconnect replaces the channel without cache invalidation at [tunnel.go:209](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/edge/tunnel.go:209). |
| + |
| + **Fix:** define how the hub determines and pins the destination’s geo capability before cache matching, keeping app headers, variants and analytics consistent. Cover reconnect and fleet-selection races. Test plaintext→TLS, TLS→plaintext and mixed-transport fleet fallback with an app varying on country. |
| + |
| +2. **P2 — §§4.1–4.2: console crashes leave staged uploads without cleanup.** Cleanup on handler or job exit does not run after abrupt process termination. A crash during staging or publication can leave up to 1 GiB under the console’s data directory. Repeated crashes and retries accumulate abandoned files despite the single-upload slot. |
| + |
| + **Evidence:** §4.1 specifies exit-path cleanup; §4.2 transfers that responsibility to the job. The reused registry stores jobs only in process-local maps at [apisystemupdate.go:374](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/geoip-for-apps/internal/console/apisystemupdate.go:374); its sweep removes finished map entries, not abandoned files. |
| + |
| + **Fix:** use a dedicated GeoIP staging directory and remove abandoned staging directories at console startup, before admitting uploads or starting the feed. Test restart and retry after abrupt termination during both staging and job execution. |
| + |
| +Verdict: revise. |
| \ No newline at end of file |