nigig-org/nimanyatta/PLAN.md

nimanyatta — Execution Plan

Completed Phases

Phase 0 — Baseline Hygiene (commit 3908dbc)

• .gitignore for target, logs, .env, concatenated dumps, .bak
• Deleted: src/database.rs, .bak/.new files, 11 concatenated_code.txt, old/, instruments_output_*, runtime logs, stray binaries
• broadcast_server.rs: removed test modules, ~470 lines dead code, dead traits
• Wrote real README with API reference, env vars, cargo aliases
• KNOWN_ISSUES.md created from prior review findings

Phase 1 — Bug Fixes (commit 39401c1)

• 500-vs-409 fix: is_duplicate_error() shared by all three error mappers in db.rs
• Session eviction fix: register_token_conn called after password login + token restore
• In-memory DB safety: DATABASE_PATH required when ENVIRONMENT=production

Phase 1 completion + Phase 2 (commit 73fa54a)

• Device revocation fix: is_token_revoked now checks device:<device_id> markers; all 4 callers updated
• Dead code removal (-1,513 lines):
  • chat_state.rs (orphaned, not a module)
  • connection_multiplexer.rs + connection_pool.rs (never instantiated)
  • io_backend/ directory (not a module, references nonexistent ServerError)
  • error.rs: removed NigigError parent, 7 dead sub-sets, 5 dead From impls — kept only IggyError
  • Dead pub use exports from optimized/mod.rs

Phase 3 — Architecture Cleanup (commit 75b2b41, -1,058 lines)

• Deleted distributor infrastructure: BroadcastDistributor, AdaptiveBroadcaster, ShardedBroadcast, BackpressureConfig — all vestigial (hot-path SendGroup already bypassed via get_sender() → try_send())
• Removed all distributor fields, construction, register/unregister calls, broadcast_rx select arm, is_under_pressure gate
• Replaced log_direct_drop with local static counter + inline function

Phase 4 — Request-Path Improvements (commit 32d237f, -38 lines)

• Registration rate limiting: added IP+username rate limit to POST /auth/register (was unprotected)
• Custom roles ownership check: replaced 4x full-table get_room_custom_roles().iter().any() scans with bounded role_belongs_to_room() query (LIMIT 1)
• Dead code: deleted get_active_devices_for_user() + ActiveDevice/ActiveDeviceRead structs (zero callers)
• Skipped token validation caching: in-memory PasetoManager cache is only populated at startup; runtime revocations write to DB but don't update the HashMap. Query is fast (in-memory SurrealDB, LIMIT 1).

Phase 5 — Security Hardening (commit ee115ae, +59 lines)

• X-Forwarded-For IP filtering: added PROXY_TRUSTED_IPS env var; middleware now checks TCP peer IP against trusted set before reading headers
• WebSocket Origin validation: added WS_ALLOWED_ORIGINS env var; middleware validates Origin header on WS upgrade, rejects with 403 if not in allowed list
• UsernameOnly auth: already properly gated behind #[cfg(debug_assertions)], rejected in release builds — no change needed

Phase 6 — Observability & Deployment (commit b08ad8c, +33 lines)

• OTEL gating: added ENABLE_OTEL env var (default false); when disabled, skip OTLP exporter entirely — prevents startup hang when no collector is available
• Env var fix: read OTEL_EXPORTER_OTLP_ENDPOINT first, fall back to OTLP_ENDPOINT
• /ready endpoint: Kubernetes readiness probe returning 200 when running, 503 during shutdown; added to PUBLIC_PATHS
• Cleanup: removed dead health() function and commented-out route from start_xitca.rs

Phase 7 — Request-Path Perf + Shutdown (commit fcaec35, 8cc54e8)

• Token validation caching (commit fcaec35): removed the per-request is_token_revoked DB query from all 4 call sites (auth middleware, WS handshake, refresh endpoint, gateway token restore). Revocation is fully covered by the in-memory PasetoManager cache, which is populated at startup and updated by every revoke_token / revoke_all_user_tokens call (revoke_tokens_for_device has zero callers). Deleted dead Database::is_token_revoked.
• Unbounded query caps: added safety caps + warnings to get_room_member_ids (LIMIT 2,000) and get_user_joined_rooms (LIMIT 1,000) so huge rooms/users degrade with a logged warning instead of unbounded memory use.
• Stale bin config: removed [[bin]] entries for deleted sources (server, websocket_gateway_iggy, load_tester, simple_ws_server) and their broken cargo aliases — the full --features b_server build was failing.
• Graceful shutdown (commit 8cc54e8): disabled xitca's built-in signal handling (SIGINT mapped to an immediate force stop that defeated our sequencing), configured shutdown_timeout, drive a graceful stop(true) from broadcast_server, wait for SIGINT or SIGTERM, and only force-abort if the drain exceeds SHUTDOWN_TIMEOUT_SECS.

Remaining / Future Work

Test Coverage Plan

1. Install cargo-llvm-cov and run with --features b_server — ✅ installed (v0.9.0)

2. Added integration tests (commit b9e5178):

   • DB write path: revoke_token, revoke_all_user_tokens, revoke_tokens_for_device — each asserts idempotence of the UPSERT (single row) plus marker/user fields
   • cleanup_expired_revocations removes only expired rows
   • load_revoked_tokens excludes expired rows
   • create_duplicate_user_returns_conflict exercises the 409 mapping with a real in-memory SurrealDB (Database::connect_fresh)
   • Config guard: production without DATABASE_PATH and without PASETO_KEY rejected; production with both accepted (env-mutating tests serialized under a lock)
   • is_duplicate_error edge cases + rate limiter — already present

3. Exclusions — no Makepad/generated code is part of the b_server build (it lives in separate crates), so nothing to configure there. Recommended command (excludes the stray <WORKSPACE> debuginfo path emitted by the ../xitca-web path-dependency macros):

   cargo llvm-cov --features b_server --bin broadcast_server \
     --html --ignore-filename-regex '<WORKSPACE>|/target/'
   

   Baseline (46 tests): total 11.82% lines — pure-logic modules run 22–92% (sms.rs 92%, fingerprint.rs 87%, session_limit.rs 43%, shared.rs 40%, db.rs 23%); HTTP/WS handlers show 0% because they need a live server and are exercised by the load-feature stress/instrumentation suites.

Phase 7 coverage run (live, instrumented)

Ran against 127.0.0.1:8080 under cargo llvm-cov run --no-report so unit tests + live traffic merge into one profile:

• Executed chat_load_tester_consistency (5×45s, 1000 users) and chat_load_tester (6 profiles, up to 3000 users) plus a REST/curl pass (register, login, refresh, SMS request/verify, auth failures, /admin/reset, /stats, /metrics, /ready).
• Total: 45.86% lines (functions 43.79%, regions 45.64%) — up from 11.82%. Auth (auth.rs 63%), SMS (sms_auth.rs 43%), WS/real-time (realtime.rs 59%, hybrid_gateway_optimized.rs 55%), db.rs 32%, middleware.rs 65%, monitoring/observability 20–80%, pure logic (sms 97%, fingerprint 90%, session_limit 84%).
• Validated graceful shutdown (commit 8cc54e8) live: SIGTERM on the app → Graceful stopped xitca-server-worker-N for every worker, Shutting down observability, Shutdown complete., port released, exit in ~2–4s (timeout backstop not hit).
• Stale load suites — repaired in Phase 8 (see below): all seven suites (fast_integration_suite, chat_load_tester_realistic, security_suite, regression_suite, session_limit_test, enhanced_security_tests, enhanced_security) now compile under cargo check --features load --bins.
• Instrumented-debug caveat: the -C instrument-coverage build is ~10–50× slower than release (register/login 2–4 s, REST keep-alive connections hit EOF while parsing… timeouts after a burst). On this build the REST success paths for routes/rooms.rs, routes/roles.rs, routes/sync.rs remain at 0% — the two working load testers drive those operations over WS instead and reach their handlers (create/join/send messaging paths covered via WS). To close the REST 0% gaps deterministically, add in‑process route tests using the existing Database::connect_fresh pattern (build AppState + handlers against embedded SurrealDB, bind an ephemeral port) rather than timing windows against a live instrumented binary. Re-run the stress profiles against a release build for trustworthy pass/fail numbers.

Phase 7.5 — In-process route tests (closes REST 0% gaps)

Added src/routes/tests.rs (#[cfg(test)], wired via mod tests): a single rest_routes_in_process test builds the exact production service chain (App → /ws + auth/room/role/sync/monitoring routes → nimanyatta_middleware → HttpServiceBuilder::h1()), serves it on an ephemeral port over xitca_server::Builder (worker_threads 1), and drives it with reqwest + JSON bodies — no fixed port, no timing windows, no external processes.

• Dev-deps: reqwest (plain HTTP, default-features = false, features ["json"]), serde, serde_json (alongside existing xitca-web/tokio/tokio-tungstenite/futures-util). xitca-test was tried first but its build.rs needs protoc (grpc), so the harness reimplements test_h1_server's ~15 lines locally and forces a graceful stop via ServerHandle::stop(true) + join.await (dropping ServerFuture from async context panics — "Cannot drop a runtime").
• Flow covered: public /health,/ready; loopback monitoring /stats,/metrics; unauthenticated /nowhere+/api/v1/sync → 401; register (weak password → 400, duplicate → 409, wrong pw/unknown user → 401); login token extraction (tokens are nested {token, expiry} objects); refresh rotation (asserts token changed) + garbage → 401; SMS request/verify (Console provider); single-device logout (access and refresh → 401, device is deactivated); re-login → all-devices logout (both tokens → 401); group + direct room create, join + duplicate 409, send MessageContent::Text (externally-tagged), messages pagination, room roles list/create/assign/update/remove/delete, set room role, set server role (owner seeded ServerRole::ServerAdmin via Database::set_server_role since granting requires an existing admin), sync with full filter.
• Real bug found & fixed: Database::role_belongs_to_room compared id = $role_id (Surreal record id vs bare ULID string) → always false → assign/update/remove custom role 404'd. Now id = type::record('custom_role', $role_id).
• Test-run coverage now 49.01% lines (functions 43.54%, regions 46.78%) on 47 tests. The previously-0% REST handlers are now exercised: routes/roles.rs 92.86% (lines), routes/rooms.rs 50.00%, routes/sync.rs 36.09%. Combined with the Phase 7 live-traffic profile, total ≥ the 45.86% live number (test binary excludes the broadcast_server main/run path, which the live run covered at 70%).

Phase 8 — Protocol crate reconstruction, WS hardening, REST coverage, suite repairs

Context: the nigig-common dependency (../nigig-lite/crates/common) was reconstructed as a faithful, fully-tested crate (21 unit tests) mirroring the wire contract defined by crates/nimanyatta-protocol and the server's own call sites. The previously-lost shapes were recovered from the consumer code: UserProfile, DeliveryReceipt { message_id, state, timestamp, by_user } (+ sent/delivered/read constructors), DeliveryState five-state enum, MessageAck + message_ack()/group_message_with_ack() constructors, MessageDeleted { …, deleted_by }, AuthMethod::SsoCallback { access_token, user_id }, client_msg_id: String on SendGroup/SendPrivate, JoinRoomRequest { reason }, Room.created_at: DateTime<Utc> (non-optional), AppError::{DatabaseConnection{source}, db_connection(op, err), SmsSendFailed{reason}, OtpMaxAttemptsExceeded (unit), SessionLimitExceeded{limit: usize}}, From<Box<dyn Error>> for AppError, and enum helpers RoomRole/ServerRole/MembershipState::{as_str, from_str}, ServerRole: Default, PermissionSet::{merge, defaults_for_server_role, defaults_for_room_role}.

• WS origin fail-closed (production): Config::from_env now rejects an empty WS_ALLOWED_ORIGINS when ENVIRONMENT=production (startup fails with a clear message); * remains an explicit opt-out. Non-production keeps the permissive default for local dev. Tests: production_requires_ws_allowed_origins, production_with_database_path_and_key_ok (now also sets origins).
• PROXY_TRUSTED_IPS documented: README env table expanded (PROXY_TRUSTED_IPS, WS_ALLOWED_ORIGINS, CORS_ALLOWED_ORIGINS, ENABLE_OTEL, token durations) plus two explainer sections — the proxy trust model (headers only honoured for TCP peers in the trusted set) and the WS origin fail-closed model.
• Four unwired REST routes registered (src/routes/mod.rs): rooms/{room_id}/invite POST, /typing PUT, /read_receipt POST, /redact/{event_id} POST — the handlers existed and were documented in the README but never reachable (root cause of the rooms 50 % coverage ceiling).
• POST /rooms/{room_id}/read_receipt now returns 501 (AppError::HttpError) instead of 500 — REST read receipts are not persisted yet; the WebSocket MarkRead path does record receipts. README documents this explicitly.
• README API tables corrected to the real paths/methods (rooms path-param form, roles, SMS).
• Gateway KNOWN_ISSUES fixes: #1 centralised + idempotent close-path cleanup via cleanup_connection, #2 LoggedOut now sent before remove_session, #5 DRY'd eviction loop onto cleanup_connection; #3 verified already enforced (session_manager.authenticate evicts prior same-username connections on both handshake and in-band login paths, including token restore).
• Gateway receipt bug fixed: DeliveryReceipt::read/delivered call sites passed Option<UserId> where the (reconstructed) protocol wants by_user: Option<String> — now Some(username.clone()).
• Load suites repaired (cargo check --features load --bins → 0 errors):
  • fast_integration_suite, session_limit_test, regression_suite are [[bin]] targets whose bodies are #[tokio::test] functions — #[test] items are cfg(test)-gated and vanish in normal builds, so the bins had no main. Added small mains (manual multi-thread runtime / #[tokio::main]) and stripped the test attributes from the orchestrator functions.
  • All other compile breaks (enum shapes, missing JoinRoomRequest.reason, removed imports) resolved via the nigig-common reconstruction above; cargo check --features b_server --bin broadcast_server is clean except the intentional RocksDb import that requires the kv-rocksdb feature (restored for the final full build).
• Coverage tests added (src/routes/tests.rs): invite → join → 409-on-reinvite → 404-unknown-user, typing PUT (member + 403 non-member), redact (happy path, 404 unknown event, 403 non-owner), read_receipt 501, custom-role creation denial (403 without ManagePermissions), non-member send 403, messages pagination (limit/direction/invalid from → 400), sync filtered (room filter, timeline_limit, include_state), sync invalid since → 400, empty room filter → zero rooms.
• Real bug found & fixed — OTP attempt counter never incremented (db.rs::increment_otp_attempts_if_under_limit): the BEGIN TRANSACTION; … IF … THEN ( UPDATE …; true ) … SurrealQL block is a parse error (; is not a valid statement separator inside an IF block), so every wrong OTP made the query fail — and the handler's fail-closed .unwrap_or(false) masked the DB error as OTP_MAX_ATTEMPTS_EXCEEDED (429) on the first wrong attempt. Rewritten as a single atomic conditional update: UPDATE type::record('otp', $id) SET attempts = attempts + 1 WHERE attempts < $max RETURN VALUE attempts — empty result set ⇒ at-limit, and the read-check-write race is gone entirely. The handler now logs the underlying error before failing closed (so a broken query can never hide as a 429 again). Regression test: db::tests::otp_attempts_increment_round_trip (store → fetch → increment → counter persisted).
• Server-role permission baseline: PermissionSet::defaults_for_server_role(User) grants CreateRooms/SendMessages/ReadMessages/InviteUsers — room-scoped powers still come from room roles; membership checks precede permission checks in every handler.
• WS wire format aligned to postcard (follow-up to the remote's protocol-crate wire pinning): nigig-common's to_bytes/from_bytes now use postcard (they had been reconstructed with serde_json), matching nimanyatta-protocol and the pre-existing chat_load_tester_consistency suite, which already called postcard::from_bytes on ServerToClientMsg. decode_all uses postcard::take_from_bytes so a fragmented frame re-buffers and bytes trailing a complete message are preserved. Remaining server/client shape divergences (variant sets, LoginSuccess/MessageDeleted/MessageContent fields) are pre-existing and documented with a remediation path in KNOWN_ISSUES ("WS wire parity").
• Final verification: cargo test --features b_server → 51 passed, 0 failed (47 prior + OTP round-trip + WS-origin fail-closed config tests + 2 new route-coverage suites); cargo test -p nigig-common → 21 passed; cargo check --features load --bins → clean (suite mains repaired); full kv-rocksdb compile check re-verified after restoring the production feature set.

Remaining / Future Work (post-Phase-8 roadmap)

Scope docs (crates/apps/nigig-site/construction-site-app-scope.md, construction-site-app-scope2.md) Phase-3 chat gaps documented as open (not yet implemented — see KNOWN_ISSUES "Scope gaps"):

1. Channel structure per construction site (site channels auto-provisioned per project; management channel; DMs) — currently all rooms are flat, user-created.
2. Pinned document library per room/site (pins exist in the permission model; no pin storage/REST yet).
3. Read receipts on documents (message receipts land via WS MarkRead; document-level receipts need schema work).
4. Mentions / search / broadcast / moderation (kick/ban enforcement) / retention policies — permission bits exist (PinMessages, KickUsers, BanUsers); no handlers.
5. Offline queue — inbox structures exist in nigig-common (InboxError); server-side offline inbox not wired.

Non-scope-gap follow-ups:

• X-Forwarded-For without trusted proxy: requests from untrusted peers keep the raw TCP peer IP (headers ignored) — documented in README; a real reverse proxy must set PROXY_TRUSTED_IPS.
• cargo llvm-cov re-run after Phase 8 to refresh the coverage numbers (rooms/sync percentages will rise with the newly registered routes + tests).