docs(19.7-06): complete lane A gap-closure plan in pier
This commit is contained in:
Jakub Zych
189
.planning/phases/19.7-generic-mcp-module/19.7-06-SUMMARY.md
Normal file
189
.planning/phases/19.7-generic-mcp-module/19.7-06-SUMMARY.md
Normal file
@@ -0,0 +1,189 @@
|
||||
---
|
||||
phase: 19.7-generic-mcp-module
|
||||
plan: 06
|
||||
subsystem: api
|
||||
status: complete
|
||||
gap_closure: true
|
||||
tags: [pier, mcp, sessions, redis, lua, concurrency, dispatch, pact, cooler, gap-closure]
|
||||
|
||||
requires:
|
||||
- phase: 19.7-02
|
||||
provides: modules/pier (catalog, dispatch, identity, sessions, fan-out, stateless path)
|
||||
- phase: 19.7-05
|
||||
provides: the updater pattern (1111828), helpers_test.go fakes, scripts/check-phase19.7.sh
|
||||
provides:
|
||||
- session reaper (reap, reapOnce) started at Mount, `expired` and `ephemeral` audit labels, GET stream ends when the record is gone
|
||||
- unexported reserver (Reserve, Release) on the memory and Redis stores (Lua script), used by serveNew
|
||||
- lock-free server build in serveNew, lookup rehydration and stateless getServer
|
||||
- Identity.fingerprint and re-filtering of a cached server when a binding returns with a different identity
|
||||
- Redis store warm-up at Mount plus a non-blocking kick on every request
|
||||
- stateless subscriptions/listen bearer re-check and session close on LRU eviction
|
||||
- refusal of a personal-token subject without TokenID; corrected pact.MCPPersonalTokenResolver contract
|
||||
- path-value and path-field rules, fixed guard-failure text, UseNumber, Host and forwarding headers, embedded-field refusal, guard-list check
|
||||
- pact.MCPPrompt.Writes under the read/write ceiling
|
||||
- cooler KeyPrefix / keyPrefix doc comments separated
|
||||
affects: [19.7-07 (token-specific events build on session.go/stateless.go), 19.7-08 (Journal sets MCPPrompt.Writes), 19.7-09 (named tests pin every fix), framework v0.5.0]
|
||||
|
||||
actuals:
|
||||
tokens: 20336
|
||||
tasks: 4
|
||||
commits: 6
|
||||
plan_head_before: d18d8f0dc206392726e225c2f5cc1340d14463c1
|
||||
plan_head_after: 95f177b1e0dbc27483d4ece9dc88751cb226f2fd
|
||||
|
||||
tech-stack:
|
||||
added: []
|
||||
patterns:
|
||||
- "Unexported capability interface next to updater (reserver), checked with a type assertion, so the exported pier.Store does not change"
|
||||
- "Build outside the router lock, then insert-or-adopt under it"
|
||||
- "Identity fingerprint decides when a cached per-identity server is re-filtered"
|
||||
|
||||
key-files:
|
||||
created:
|
||||
- modules/pier/reaper_smoke_test.go
|
||||
modified:
|
||||
- modules/pier/session.go
|
||||
- modules/pier/pier.go
|
||||
- modules/pier/audit.go
|
||||
- modules/pier/store.go
|
||||
- modules/pier/store_memory.go
|
||||
- modules/pier/store_redis.go
|
||||
- modules/pier/stateless.go
|
||||
- modules/pier/identity.go
|
||||
- modules/pier/events.go
|
||||
- modules/pier/dispatch.go
|
||||
- modules/pier/catalog.go
|
||||
- modules/pier/filter.go
|
||||
- modules/pier/backend_guard.go
|
||||
- modules/pier/export_test.go
|
||||
- modules/pier/README.md
|
||||
- modules/pact/mcp.go
|
||||
- modules/pact/README.md
|
||||
- modules/cooler/cooler.go
|
||||
- docs/services/mcp.md
|
||||
|
||||
key-decisions:
|
||||
- "P-24: a reaper goroutine started at Mount closes every local session whose store record is gone every min(idle_ttl/2, 1m) (floor 1 s); the store TTL stays the single source of truth across instances; stopped through Pier.stopReap (no backpack shutdown hook yet)"
|
||||
- "P-25: atomic per-binding reservation through an unexported reserver interface; Redis uses one Lua script (prune, count, SET PX, SADD, PEXPIRE) on the existing key layout"
|
||||
- "P-26: Identity.fingerprint (sorted scopes, both subjects with guard/user/client/token/scopes, ceiling, token kind); a changed fingerprint re-runs the event add/remove diff under the session's or entry's own mutex, with context.WithoutCancel so a client hang-up cannot leave the set filtered by cancelled guard checks"
|
||||
- "P-27: path fields must be required at boot; at runtime a missing value, an empty, . or .. segment, or / in a single segment is a tool error before dispatch"
|
||||
- "P-28: pact.MCPPrompt.Writes (additive) puts a prompt under the read/write ceiling"
|
||||
- "Guard dispatch errors answer the fixed text `pier: the tool could not be run for this caller`; only pact.ErrMCPTokenInvalid and a new errSubjectUnavailable sentinel (backend user unknown or deactivated) keep their own fixed texts"
|
||||
|
||||
patterns-established:
|
||||
- "Background lifecycle goroutines of pier (reaper, Redis warm-up) share the context whose cancel func is Pier.stopReap"
|
||||
|
||||
requirements-completed: [SC-1, SC-2, SC-3, D-10, D-11, D-13, D-17, D-19, D-20, D-25, D-26, D-27, D-28, D-32]
|
||||
|
||||
duration: 23min
|
||||
completed: 2026-10-23
|
||||
---
|
||||
|
||||
# Phase 19.7 Plan 06: Lane A gap closure in pier Summary
|
||||
|
||||
This plan closes lane A of the code review in the framework's MCP module. Abandoned stateful sessions are now reaped at the store TTL. The per-binding cap is reserved atomically (memory store under its lock, Redis through a Lua script). No router-wide lock is held across guard or database I/O. A binding that returns with a wider identity is re-filtered on that request. Redis instances subscribe to fan-out when they mount. Stateless listen streams are re-verified, and an evicted stateless server closes its streams. Personal-token subjects must carry a TokenID. Dispatch only hands routes path values the HTTP mux could produce, and guard failures answer with fixed text. Prompts gain a `Writes` ceiling.
|
||||
|
||||
## Performance
|
||||
|
||||
- **Duration:** about 23 min
|
||||
- **Completed:** 2026-10-23
|
||||
- **Tasks:** 4 of 4
|
||||
- **Files modified:** 20 (1 created)
|
||||
|
||||
## Commits and findings
|
||||
|
||||
| Commit | Task | Findings |
|
||||
|--------|------|----------|
|
||||
| 2bf9dca `fix(pier): reap stateful sessions whose store record expired` | 1 (tracer) | A-CR-01, A-IN-01, A-IN-06 |
|
||||
| c1c0c77 `fix(pier): reserve session slots atomically and build servers outside router locks` | 2 | A-WR-03, A-WR-07 |
|
||||
| 2404572 `fix(pier): widen returning identities, warm the Redis store, end stateless listen streams, refuse personal tokens without a TokenID` | 3 | A-WR-04, A-WR-05, A-WR-06, A-WR-09, A-IN-03 |
|
||||
| 104db99 `fix(pier): refuse path values the mux never delivers and harden dispatch` | 4 | A-WR-01, A-WR-02, A-WR-08, A-IN-04, A-IN-05, A-IN-07, A-IN-08 |
|
||||
| 211cec1 `fix(pact): add MCPPrompt.Writes so a write-directing prompt sits under the read/write ceiling` | 4 | C-IN-01 (framework part) |
|
||||
| 95f177b `docs(cooler): give KeyPrefix and keyPrefix their own doc comments` | 4 | A-IN-02 |
|
||||
|
||||
All lane A findings are covered: A-CR-01, A-WR-01..09 and A-IN-01..08.
|
||||
|
||||
## Accomplishments
|
||||
|
||||
- **Reaper (Task 1).**
|
||||
- `sessionRouter.reap` and `reapOnce` run from `Mount` and close each local copy whose record has expired, logging `expired`.
|
||||
- A store error keeps the copy and is logged once per pass.
|
||||
- `serveStream`'s re-check ends the stream and closes the local copy when the record is gone.
|
||||
- Ephemeral tool calls log the session as `ephemeral`.
|
||||
- `TestPierReaperSmoke` proves the flow with a real SDK client. New seams: `ReapNow`, `LocalSessions`, `StopReaper`.
|
||||
- **Reservation and locks (Task 2).**
|
||||
- `reserver` with memory and Redis `Reserve`/`Release`, plus `lazyStore` forwarders.
|
||||
- `serveNew` reserves first, then connects, and releases the slot on a connect or initialize failure.
|
||||
- `serveNew`, `lookup` rehydration and the stateless `getServer` build the server with no `sr.mu`/`sp.mu` held, then insert or adopt.
|
||||
- **Identity and lifecycle (Task 3).**
|
||||
- `fingerprint()` is stored on `localSession` and `statelessEntry`; the `widen` helper re-filters on a change.
|
||||
- `lazyStore.warm` (backoff 1 s to 30 s) starts at Mount, and `kick()` runs from `route`.
|
||||
- The stateless `ServeHTTP` re-checks the bearer every `recheck` for `subscriptions/listen` POSTs.
|
||||
- LRU eviction closes the evicted server's sessions.
|
||||
- `resolvePersonalToken` refuses `TokenID == 0`, and the resolver contract text is rewritten.
|
||||
- **Dispatch hardening and prompt ceiling (Task 4).** Covers every item listed in the commit table, and `allowsPrompt` honours `decl.Writes`.
|
||||
|
||||
## Verification
|
||||
|
||||
- `go vet ./...` and `go vet -tags summercms_mcp ./...` are clean.
|
||||
- `go test -count=1 -short ./...` is green, and so is `-race` on `./modules/pier/...`.
|
||||
- `go test -count=1 ./modules/pier/... ./modules/surf/...` passes with Docker available.
|
||||
- `cd examples/hello && go test ./...` is green.
|
||||
- `scripts/check-phase19.7.sh --go`, `--docs` and `--marker` all PASS.
|
||||
- `summer docs:build --check` reports no problems.
|
||||
- `go.mod`, `go.sum` and `admin/package-lock.json` are unchanged since 2527498.
|
||||
- Commit 104db99 was checked on its own in a temporary detached worktree (vet, short tests, docs check), because Task 4 was split into three commits.
|
||||
- Ad-hoc checks were run and then deleted, since plan 09 owns the named tests:
|
||||
- 30 concurrent initializes with `max_per_binding: 2` gave exactly 2 OK and 28 429 on both the memory store and miniredis.
|
||||
- A read grant followed by a read+write grant on the same binding gained the write tool on both the stateful and the stateless path.
|
||||
- `9007199254740993` survived as a path value and as a query value.
|
||||
|
||||
## Deviations from Plan
|
||||
|
||||
### Auto-fixed Issues
|
||||
|
||||
**1. [Rule 2 - Missing critical] Sentinel for the backend guard's "unknown or deactivated" answer**
|
||||
- **Found during:** Task 4 (A-WR-08)
|
||||
- **Issue:** The plan says to keep specific messages for "the provider sentinel errors dispatch.go already distinguishes", but dispatch.go distinguished none. The backend guard's "unknown or deactivated" answer was a plain `fmt.Errorf`.
|
||||
- **Fix:** Added `errSubjectUnavailable` in `modules/pier/backend_guard.go`, which the backend guard now wraps. Dispatch maps it, and `pact.ErrMCPTokenInvalid`, to fixed texts. Every other error gets the generic fixed text and is logged server-side.
|
||||
- **Files modified:** modules/pier/backend_guard.go (not in the plan's file list)
|
||||
- **Commit:** 104db99
|
||||
|
||||
**2. [Rule 2 - Missing critical] Widening uses a non-cancellable context**
|
||||
- **Found during:** Task 3 (A-WR-04)
|
||||
- **Issue:** `catalog.allowed` hides an item on any guard error. If the request context were cancelled mid-recompute, the server would be re-filtered with tools missing and the new fingerprint stored, so the narrower set would stick.
|
||||
- **Fix:** `widen` calls `allowed` with `context.WithoutCancel(ctx)`.
|
||||
- **Files modified:** modules/pier/events.go
|
||||
- **Commit:** 2404572
|
||||
|
||||
**3. [Rule 3 - Blocking] The ephemeral label survives `sessionPrefix` and the SDK session id**
|
||||
- **Found during:** Task 1 (A-IN-06)
|
||||
- **Issue:** An ephemeral call's SDK session id is `""`, the same as a stateless call. A plain label would also have been cut to 8 characters by `sessionPrefix`.
|
||||
- **Fix:**
|
||||
- `serveEphemeral` marks the per-request token info's `callInfo.session` as `ephemeral`.
|
||||
- `toolHandler` overrides `info.session` only with a non-empty SDK session id.
|
||||
- `sessionPrefix` passes the `ephemeral` label through untruncated.
|
||||
- **Files modified:** modules/pier/session.go, modules/pier/audit.go
|
||||
- **Commit:** 2bf9dca
|
||||
|
||||
**4. [Docs rule] Doc updates outside the listed tasks' file sets**
|
||||
- In Task 3, `docs/services/mcp.md` gained the resolver/TokenID sentence plus one sentence each on the reaper and on widening. The CLAUDE.md rule requires docs to change in the same commit as an exported contract change.
|
||||
- Task 4 was split into three commits (pier hardening, pact `Writes`, cooler docs) so each is one logical change and carries the commit-type prefix the success criteria name.
|
||||
|
||||
### Tests changed for old behaviour
|
||||
|
||||
None. No existing test relied on the old behaviour: no fixture uses an optional path field, a shared `pat:user:0` binding, an embedded GET/DELETE field or raw dispatch error text. Every regression pin passed unchanged.
|
||||
|
||||
## Notes for later plans
|
||||
|
||||
- Behaviour change for consumers: Mount now starts two background goroutines (the reaper, and for the Redis store a warm-up that dials `redis.url` with a backoff). `route:list` and tests on the Redis store therefore dial in the background. The pier README and Mount's doc comment say so.
|
||||
- Consumer impact of the new boot errors: no sibling repository (fonoteka.go, sm-*-app, sm-journal-plugin, the user plugin copies) declares a path field with `omitempty`/`omitzero` or an embedded struct on a GET/DELETE input. Plan 09's consumers stage confirms this by building them.
|
||||
|
||||
## Known Stubs
|
||||
|
||||
None.
|
||||
|
||||
## Self-Check: PASSED
|
||||
|
||||
- FOUND: modules/pier/reaper_smoke_test.go
|
||||
- FOUND: 2bf9dca, c1c0c77, 2404572, 104db99, 211cec1, 95f177b
|
||||
Reference in New Issue
Block a user