DO NOT MERGE YET - fix(tracker): name the reason an untrack happened, and make the admin Untrack button mean something - #148
Draft
guarzo wants to merge 94 commits into
Draft
DO NOT MERGE YET - fix(tracker): name the reason an untrack happened, and make the admin Untrack button mean something#148guarzo wants to merge 94 commits into
guarzo wants to merge 94 commits into
Conversation
The corporation typeahead in map settings did nothing at all. The cause was not in the LiveComponent: commit e791d4e fixed `is_access_token_expired?/1` to report a nil `expires_at` as expired, which routes exactly those characters into `handle_refresh_token_result/5` — where five `DateTime.from_unix!/1` calls, used only to compute a `time_since_expiry` log field, raised FunctionClauseError on the same nil. That killed every authenticated ESI call for such a character, and the component's rescue rendered it as an empty dropdown. Guard the diagnostic with `time_since_expiry/1`, which degrades to nil rather than raising: metadata for a log line must never be what fails a request. Then stop the failure from being invisible. `Character.search/2` collapsed ESI errors into `{:ok, []}`, so no caller could distinguish "search failed" from "no matches". It now returns `{:error, reason}`, and all three callers are updated: the notifications tab and the ACL member search surface a message, the map-systems handler logs while keeping its `%{results: []}` reply shape (the JS consumers read only `.results`). `CorporationSearch.search/3` no longer reports success for an unusable `characters` argument, which is how an `%Ash.NotLoaded{}` association produced a permanently empty dropdown with nothing in the logs. Also fix the reported UI issues: buttons stretched to full width (`self-start`), misaligned labels (`minmax(0, 24rem) auto` grid columns), and the `btn-error` class, which is inert in this theme without `.btn` (`p-button-danger`).
…hardening, codegen drift (#102) * chore: clear five pre-existing compile warnings Removes @c4_system_class and @ns_system_class, stranded when c4b653f moved the wormhole class list to WandererApp.Map.SystemClass. Drops an unused binding in get_connection_info/2, uses the existing UserActivityItem alias, and migrates the Gettext backend to Gettext.Backend across its three call sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: stop cancel_ping misses being swallowed by a debug catch-all The clause commented 'Catch-all for cancel_ping to debug why it doesn't match' ignored both event and assigns and returned {:noreply, socket}, so a cancel rejected for insufficient permission or no tracked characters was indistinguishable from a successful one. Removing it lets misses reach the core handler's unhandled-event log. Follow-up, deliberately not in scope: cancel_ping has no user-facing feedback clauses for those cases, where add_ping does. This also clears the last --warnings-as-errors failure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: least-privilege token and SHA-pinned actions in release workflow The workflow granted contents: write at the workflow level, so every step inherited that scope while holding Docker Hub credentials. Mutable action tags in that context let an action owner change what runs. Scopes contents: write to just the tag-build-deploy job that pushes the tag and pins all third-party actions (checkout, docker/login-action, docker/build-push-action, appleboy/ssh-action) to commit SHAs resolved from the upstream repos, with the version kept as a trailing comment. DOCKER_HUB_PASSWORD is referenced only in the docker/login-action step and is never echoed, written to a file, or passed as a build arg -- verified, no change needed there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore: realign Ash resource snapshots with the live schema `mix ash.codegen --check` failed with five pending files, but the four columns it wanted to add already exist. They were created by hand-written migrations -- 20260209100000_add_intel_source_map_id, 20260209100001_add_inherited_from_map_id and 20260425000000_add_map_connection_locked_by -- and the resource snapshots were never regenerated afterwards, some of them for over a year (map_system_structures_v1 was last snapshotted 20250116211927). Because codegen diffs resources against snapshots rather than against the database, the migration it generated would have run `add :intel_source_map_id` and friends on tables that already have those columns, failing on every existing database including production. Only the snapshots are committed here; the generated migration was discarded. No DDL ships and no database is touched -- this is purely bookkeeping, and it lets `mix ash.codegen --check` be used as a CI gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: create the map_chain_v1 locked_by_id foreign key `MapConnection` declares `belongs_to :locked_by, Character`, and the resource snapshot records `map_chain_v1_locked_by_id_fkey` accordingly — but the constraint exists in no database. The migration that introduced the column, 20260425000000, added a bare `:binary_id` with no `references(...)`. That gap is self-concealing: because the snapshot asserts the constraint, `mix ash.codegen` considers it already applied and will never generate it. Without this migration the previous commit would make the drift permanent. Backfill is a no-op by construction. Connection locking writes `locked_by_id` only into the `map_<id>:conn_<id>:locked_info` cache entry (map_server_connections_impl.ex:321), never into the column, so every row is NULL — verified zero orphans before adding the constraint. `on_delete` is left unset to match the resource: destroying a Character that holds a lock should raise rather than silently drop the reference. * chore: address review findings on the gettext and release.yml changes - `gettext.ex` moduledoc still showed the removed `import WandererAppWeb.Gettext` call site; it is now the `use Gettext, backend:` form the backend requires. - `mix.exs` allowed `{:gettext, "~> 0.20"}` while the code needs `Gettext.Backend`, which does not exist before 0.26. Raised the floor to match the lock file (0.26.2). - `release.yml`'s comment described the write scope as step-level; GitHub Actions has no step-level `permissions`, and it is set on the job. - Dropped a test comment's hardcoded line number, which was already stale. `persist-credentials: false` was also suggested and deliberately not applied: the "Create a tag" step pushes with plain `git push origin`, which depends on the credential `actions/checkout` persists. Setting it false breaks releases. * fix: repair the maps_v1.scopes column default Two migrations set the default with the charlist literal `~c"{wormholes}"`: 20260331192521_add_mass_to_map_chain_passages.exs and 20260406213852_add_character_description.exs. Ecto rendered the charlist as a list of code points, so the live default was ARRAY['123','119','111','114','109','104','111','108','101','115','125'] — the characters of `{wormholes}` as eleven separate elements — where api/map.ex:17 and the snapshot both intend the single-element `{wormholes}`. This was assumed to be latent, on the grounds that `Api.Map` declares `default([:wormholes])` and so every Ash write supplies scopes explicitly. It was not. `SlugRecoveryTest`'s `insert_map_directly/4` inserts through raw SQL without a scopes value, and reading such a row back through Ash fails casting '123' against the `{:array, :atom}` `one_of` constraint. That is why "automatically recovers and retries when duplicates are found" was failing, and it had been written off as an unrelated pre-existing failure. It passes now. Any raw insert, seed, or manual INSERT during an incident hits the same thing, and scopes drives connection validity. The two source migrations are left untouched: they are applied everywhere, so editing them fixes nothing this does not, and applied migrations are history. * fix: stop the ACL member create path serializing raw Ash errors (CWE-209) `json(%{error: "Creation failed: #{inspect(error)}"})` and its sibling on the entity-lookup branch returned the whole error to the caller. `inspect/1` on an Ash error carries the changeset: resource module, internal field names, and the attributes that were submitted. PR #101 closed the same leak on the lookup path via `with_membership/4`; these two were missed. Validation errors are still returned, because "role is invalid" is actionable and swallowing it would make the endpoint unusable — but through `validation_messages/1`, which has no `inspect/1` fallback: an error struct it does not recognise yields the fixed string "Invalid value". Everything else is logged server-side and answered generically. `validation_messages/1` is public with @doc false so it can be unit tested. `WandererApp.Esi` hard-delegates to `Esi.ApiClient` (esi.ex:9), so the create path cannot be driven end-to-end without network access. * perf: cache the per-map ready-character set behind one repo helper The ready set was computed by two byte-identical private functions — `TrackingUtils.all_ready_character_eve_ids/1` and `MapCharactersEventHandler.get_all_ready_characters_for_map/1` — each doing a `get_by_map` plus flat_map/uniq. The event-handler copy runs inside `map_ui_characters_with_ready/2`, which every connected LiveView calls while enriching the same `characters_updated` broadcast, so a map with N viewers did N identical queries per character update. Both are replaced by `MapUserSettingsRepo.ready_character_eve_ids/1`, cached for 5 minutes. A read failure returns `[]` for that call but is deliberately not cached: caching it would pin every viewer's ready flags off for the whole TTL after one transient database error. Invalidation hangs off the `:update_ready_characters` action rather than its four call sites, so a fifth write path cannot forget it. It is an `after_transaction` hook per the CLAUDE.md invariant — an `after_action` hook would drop the cache on a write that then rolls back, and the action needs `require_atomic? false` to carry a non-atomic change. * perf: select the connection count from the ReactFlow store `useEdges()` returns a new array identity on every edge-store change, so every zoo node re-rendered whenever any edge anywhere on the map changed — dragging one connection re-rendered the whole canvas — even though each node only wants its own degree. `useStore` with a memoized selector returning a number re-renders only when that node's count actually changes. Same pattern as SolarSystemEdge.tsx:45. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…104) The two "Add" buttons in the notifications tab spanned the whole panel. `grid-template-columns: ... auto` makes the last track absorb all leftover free space, and a grid item defaults to `justify-self: stretch`, so each button filled 768 - 24(p-3) - 384(input) - 8(gap) = 352px. Measured against the reported screenshot the buttons were exactly that wide. `self-start` was added to the buttons in the flex containers in #103 and worked there, but it sets `align-self` — the cross axis — so it could never have constrained width in these two grid forms. Sizing the second track as `max-content` fixes it at the source. Kept as an inline style rather than a `justify-self-start` class so it does not depend on a Tailwind rebuild. The corporation lookup still fails at runtime after the FunctionClauseError fix in #103, and the banner rendered the same generic sentence for every cause, so the only thing that identifies the failure lived in the server logs. Render the reason too: `:forbidden` (re-authorise the character) and `:error_limited` (wait it out) call for opposite responses from the user. This is a map-admin surface and the reasons are ESI status atoms. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… producers (#105) * feat(external-events): include system_id in add_system and deleted_system payloads The MapSystem record UUID was already in scope at all three broadcast sites but never sent, leaving JSON:API consumers with no resolvable identity for system events. Additive only - no existing key is renamed or removed. * fix(json-api): correct system event clauses and add payload helpers System clauses read system_id/x/y, which no producer sends. Read the real keys, stringify identifiers per JSON:API, and add struct-safe payload access. * fix(json-api): correct signature clauses and add signatures_updated The producer sends the EVE signature code, not a record UUID, so these events now use the event ULID as identity. Adds the missing signatures_updated clause, which previously fell through to the generic events fallback. * fix(json-api): correct connection clauses Read solar_system_source_id/solar_system_target_id as the producers send them, and expose them as attributes rather than unresolvable map_systems relationships. * fix(json-api): stop character clauses raising on struct payloads Character events broadcast Api.Character structs, which do not implement Access, so bracket access raised. Read via Map.fetch and project through an explicit allowlist so OAuth tokens can never reach the wire. Adds characters_updated, which emits an array and previously fell through to the generic fallback. * fix(json-api): correct acl member and rally point clauses ACL clauses read character_eve_id/character_name/access_list_id; the producer sends eve_id/member_name/acl_id. rally_point_removed reads rally_point_id; that producer sends :id. * fix(json-api): emit one kills resource per killmail in a batch The :map_kill clause read per-kill keys off the batch payload, so every field was null and a batch of N kills collapsed to one empty resource. data is now an array built from the killmails list, and the kill-count variant - which sends no killmails key - keeps its count instead of being discarded. * docs(external-events): correct broadcast examples to real producer shapes The @doc examples documented the formatter's invented key names rather than what producers send, which is how the drift propagated. Adds module-wide tests asserting string identifiers, no EVE ids in relationship slots, no fallthrough to the generic events resource, no all-null attribute maps (the original bug's signature), and no token leakage across every event type. Also corrects three stale file:line citations in the test file (map_server_systems_impl.ex broadcast lines drifted after Task 1's edits) and makes the characters_updated credential-leak test symmetric with its siblings by refuting forbidden field names as well as sentinel values. * fix(external-events): send system_id from the third add_system site The MapSystemRepo.upsert/1 branch of do_add_system_from_location was the only add_system broadcast still omitting system_id, so that path emitted the system_events ULID fallback while its two siblings emitted map_systems/UUID. system.id there is the uuid_primary_key of the upserted record - the same guarantee the siblings rely on. Adds a source-level contract test that parses the producer and asserts every add_system/deleted_system broadcast literal carries system_id plus every key it sent before. The formatter's own tests are fed fixtures, so nothing else would notice this line being dropped again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(json-api): skip null-id killmails, close invariant gaps, correct citations Formatter: - map_kill drops killmails whose killmail_id is nil. validate_flat_format_kill/1 checks required fields with Map.has_key?/2, so a present-but-nil id reaches the wire as "id": null - invalid JSON:API. Fabricating an id from the event ULID would collide across the batch, so the element is omitted instead. - determine_action/1 reports :characters_updated as "bulk_updated", matching its sibling :signatures_updated instead of falling through to "unknown". - the generic fallback builds its map relationship with map_relationship/1 like every other clause (drift removal; the clause is unreachable today). Tests: - the "no entirely-null attributes map" invariant now drops formatter-injected timestamps before the check. Those are never nil, so they were satisfying it single-handedly for 14 of 21 fixtures - i.e. it was vacuous for exactly the clauses the original bug hit. - the fixture list is asserted to cover Event.supported_event_types/0, so a new event type cannot silently opt out of every invariant. - added the map_kill count-only fixture, the other shape that clause renders. - added coverage for a batch mixing a valid and a nil-id killmail. Docs: corrected producer citations after the third add_system site shifted line numbers, dropped a signature_added miscitation, and made the ExternalEvents moduledoc example match the real add_system payload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * refactor: inline the single-use has_key?/2 helper Polish pass follow-ups, both non-behavioural: - has_key?/2 had exactly one call site. Inline it at that site so the key-style tolerance is visible where it matters instead of behind a two-line abstraction. - Drop a fixture count from a test comment. It said 14; the actual number is 15, and any number there drifts the moment a fixture is added. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: refresh producer file:line citations after the zoo rebase The zoo branch inserts ~37 lines above every system-event broadcast site and shifts the connection and character producers too, so every citation in the formatter and its tests drifted. The contract test asserts via the AST, not by line, so nothing would have failed - the citations would just have quietly started pointing at the wrong code. Re-derived in-tree; unchanged ones (signatures, pings, ACL, the two kill broadcast sites) left alone: systems 348 -> 385, 636 -> 673, 690 -> 729, 902 -> 943, 1119 -> 1187 connections 748 -> 779, 1083 -> 1104, 1140 -> 1161 characters 1030 -> 1037, 1041 -> 1048, 485 -> 486 kills 296-346 -> 298-350; the required-field check moved out of validate_flat_format_kill/1 into validate_required_fields/2 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The search runs as the FIRST of the user's characters and only that one, so a single stale token breaks the feature while every other character is fine. 'Re-authorise a character' is then misleading: re-authorising any of the others changes nothing. Character selection now goes through CorporationSearch.search_character/1 so the message and the request cannot disagree about which character was used. The name is added to the rendered banner and to the log lines in both Character.search/2 and the notifications component. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ent verdict (#107) Three routing corrections plus the docs and UI copy that go with them. 1. `:unknown` verdict. `Matcher.involvement/3` could not previously distinguish "this kill is not ours" from "we could not tell". Both came back as `:not_involved`, and `Router` treats that as permission to apply the excluded-system and wormhole-only filters. So a Cachex outage or a map that was briefly not running silently dropped every k-space kill on every map with the default `wh_only`. `tracked_eve_ids/1` now returns `:unavailable` rather than an empty MapSet, and the undetermined case routes to the system webhook with the filters bypassed. 2. Flat payloads. A payload with no attacker keys at all (absence is the codebase's own "we don't know who attacked" signal) hit the same silent drop. It is now `:unknown` too, and the diagnostic went from `Logger.debug` to a throttled `Logger.warning` — this is the reason a kill lands in the system channel instead of the character one, so it should be visible without turning on debug logging. 3. Corporation filter semantics. `focus_corp_ids` widened the character channel: map-tracked characters OR filtered corporations. The intent is replacement — when the filter is set it answers "who is the character channel for" on its own, so setting it can take untracked pilots out of that channel. Empty means map-tracked characters, i.e. today's behaviour, so no migration and no config change. Kills matching the active criterion still bypass both filters. Relabelled "Focus corporations" -> "Corporation filter" with copy that says what it does. Also fixes the Add-button vertical offset: `CoreComponents.live_select/1` always rendered an empty daisyUI `.label` spacer row above the input, making the wrapper taller than the input, so `items-end` on the row aligned the button to the wrapper rather than the field. Added an opt-out `label_row` attr (defaulting to true, so no existing call site moves) and switched the two notification forms to `items-start`. The `:killmail` factory claimed to mirror `adapt_nested_format_kill/1` but omitted the attacker id lists, so every routing test was accidentally exercising the undetermined path. Fixed, with a note that their absence is load-bearing for the tests that do want it. Blog post updated with the full routing table, the undetermined case, and the replacement semantics. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…#108) * fix: remove LiveSelect's empty tags container from the notification add rows The Add buttons were still sitting high after the label-row fix. The label rows were a real contributor but not the whole story: LiveSelect's template (`component.html.heex:11`) renders its `tags_container` unconditionally — including in `:single` mode, where it can never hold a tag — and both bundled styles give it `flex flex-wrap gap-1 p-1`. That is an always-empty element contributing 8px directly above the text input, in every single-mode select. Collapsed `label_row` into one `compact` attr, since all three pieces of chrome (top spacer label, empty tags container, error label row with no errors) exist for the same reason and a caller that wants one wants all three. Still defaults to false, so the other four call sites in the app are byte-identical — covered by a regression test. Suppressing the tags container is gated on `mode == :single`; in the tag modes that element holds the selection. Uses `tags_container_class` (override) rather than `_extra_class` (append), so hiding it does not depend on whether Tailwind emits `hidden` after `flex` in the stylesheet. The rows are now `items-stretch` rather than `items-start`: with `compact` the wrapper is exactly the input's height, so stretching gives the button the field's exact box instead of relying on the two having equal natural heights. Verified by rendering the component and asserting on the markup rather than by eye — three tests in test/unit/components/live_select_compact_test.exs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix: match the Add button's box to the input instead of stretching it `items-stretch` equalises the two GRID ITEMS — the field wrapper and the button — but the input does not fill its wrapper (LiveSelect nests it inside a plain div with no height). So a button that is naturally taller than the input just stretched the wrapper around a short input, and read as an oversized button rather than a misaligned one. Match the box by construction instead. `.p-inputtext` is font-size 1rem + 0.5rem vertical padding + 1px border; `!py-2 !text-base` gives the button the same three values, so the two boxes are equal by the same arithmetic rather than by negotiation through the wrapper. The `!` is needed because `.p-button.p-button-sm` (0.875rem / 0.4375rem) is a two-class selector. Back to `items-start`: with `compact` the wrapper is exactly the input's height, so the columns already start at the same y, and not stretching means neither element can inflate the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Revert "fix: match the Add button's box to the input instead of stretching it" This reverts commit 5c0123d. * fix: pin line-height so the Add button and its input have the same box Measured from a screenshot of the deployed build rather than guessed at: input 40px tall, button 47px, tops aligned. Subtracting the known terms leaves the button's content box at 31px for 14px text against the input's 22px for 16px text — same font family, so the discrepancy is line-height, which nothing in either theme sets and which the two elements therefore inherit differently. That is why matching font-size and padding alone did not converge, and why raising the button's font-size in the reverted commit made it taller still. So match every height term, not just the visible ones: font-size (`!text-base` = the input's 1rem), vertical padding (`!py-2` = its 0.5rem), border (already 1px on both), and line-height (`leading-normal` on the button AND on the input, removing the inheritance asymmetry). Back to `items-start`. With the boxes equal, stretch is a no-op; if the two are still a pixel or two apart, top-aligned with a small bottom difference degrades better than a stretched wrapper around a short input. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Revert "fix: pin line-height so the Add button and its input have the same box" This reverts commit 5584ade. * fix: size the Add button from the field's height instead of matching boxes The button and the input do not share a box model: the input is styled by @tailwindcss/forms and .p-inputtext, the button by .p-button/.p-button-sm, and three attempts to make their font-size, padding and line-height agree produced a button that was consistently taller than its input. Drop the negotiation. With the button's vertical padding removed, its intrinsic height is one line of text - always shorter than the input - so the grid row is sized by the field alone and items-stretch hands the button exactly the field's height, whatever that turns out to be. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ly.toml (#109) * docs: track the Fly.io migration spec and implementation plan Force-added past the local docs/ entry in .git/info/exclude, which is a convenience for scratch files rather than a statement that docs/ is untracked. These were kept untracked at first. An earlier draft of the spec was then lost and had to be reconstructed from a session transcript, which is a poor backup and only worked by luck. Tracking them costs nothing and makes the loss impossible; they can be dropped from the branch once the migration is done. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(config): let PHX_HOST and WEB_APP_URL override the .fly.dev derivation On Fly, FLY_APP_NAME is always set, so both env vars sat on the dead branch of their case expressions and were never read. That forced the EVE OAuth callback_url to <app>.fly.dev, making a custom domain impossible. Behaviour is unchanged when neither variable is set. * docs(plan): empty WEB_APP_URL must still raise, not fall back Task 1's review found the test file mandated treating an explicitly-empty WEB_APP_URL as unset. Today System.get_env returns "" for a set-but-empty variable, URI.parse gives a nil scheme, and runtime.exs raises with the variable named. Falling back instead would boot the app with a wrong OAuth callback -- a later, quieter failure. WEB_APP_URL= with nothing after it is what a docker-compose .env file produces, so this is a shape operators actually hit. PHX_HOST="" still defaults to localhost: it previously produced http://:8000, which is not usable by anyone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(config): let an explicitly-empty WEB_APP_URL pass through unchanged WEB_APP_URL="" in a .env file previously reached URI.parse/1 unmodified and raised at boot on the missing scheme, stopping an operator before they got a silently wrong OAuth callback URL. The resolve_web_app_url/4 guard collapsed that case into "unset", which would have swallowed the raise. resolve_host/2 is unaffected: an empty PHX_HOST has no equivalent loud-failure to preserve. * fix(kills): forward socket options to the websocket transport phoenix_gen_socket_client splits transport options on [:extra_headers, :ssl_verify] only, so :socket_opts fell through into the handler-state argument and never reached :websocket_client. That made :inet6 unsettable, and Fly's 6PN .internal addresses are IPv6-only. Also removes the connect/send/recv timeouts from client.ex. They never applied: upstream's handler init/1 reads only :keepalive, and websocket_client 1.5.0 hardcodes its connect timeout to 6000ms. They looked adjustable and were not. Gated on WANDERER_KILLS_IPV6, default false, so non-Fly deployments are unaffected. phoenix_gen_socket_client pinned because the shim depends on its private handler-state shape. * docs: document WANDERER_KILLS_IPV6 in .env.example * feat(web): add GET /health on a dedicated pipeline Returns app version and database reachability, for Fly health checks. Deliberately not in the :api scope: that pipeline includes CheckApiDisabled, which halts 403 when WANDERER_PUBLIC_API_DISABLED is set. With min_machines_running = 1, an unhealthy check kills the only machine, so a feature flag would become a total outage. The flag defaults to false, so that failure would have been latent. * docs(plan): /health reports database state, never gates on it Task 3's review flagged the plan-mandated 503-on-DB-unreachable behavior as being in tension with min_machines_running = 1: it hands Fly a kill signal for a fault a restart cannot repair, so a transient Postgres blip buys a window with zero machines serving while the database problem continues. /health now always answers 200 while the app is serving, with database state in the body for humans and dashboards. Two smaller findings from the same review are folded in, both of which only matter now that a database fault must not produce a non-200: catch :exit around the query (rescue does not cover the exit DBConnection raises when the pool is not alive, which would crash the request into a 500), and an explicit 2s query timeout so a saturated database cannot hold polls open for the Repo default 15s. Also corrects Step 2 and Step 4: /health does collide with the live "/:slug" wildcard, and wins on definition order rather than any literal-over-dynamic rule, so the scope's position in the router is load-bearing and now says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(web): stop the database from gating GET /health status code Fly kills a machine that fails its health check, and min_machines_running = 1 means that is the only machine. A restart cannot repair an external Postgres outage, so wiring the status code to database reachability would turn a transient blip (partition, pool exhaustion, slow migration) into a window with zero machines serving traffic. Database state is still reported in the body for humans/dashboards, just no longer tied to a restart. Also bounds the reachability check to 2s (the Repo default of 15s would let a saturated database hold health polls open indefinitely) and catches :exit in addition to rescuing raised errors, since a dead connection pool exits rather than raising. * chore(fly): production-shaped fly.toml Real app name, iad, shared-cpu-2x/2gb, min_machines_running = 1 with the single-machine rationale in a comment, /health check. Drops the hard-coded PHX_HOST, which would defeat the custom-domain change, and the [[metrics]] block, which scrapes :4021 while PROMEX_DISABLED defaults true so every scrape fails. Adds WANDERER_KILLS_IPV6 = "true": the wanderer-kills service is reached over Fly's 6PN private network, which is IPv6-only, and the app-side default is "false", so omitting this would make the connection silently retry forever. * fix(fly): address review round 1 on fly.toml Seven fixes from task-4-review.md, all confined to fly.toml: - Reword the WANDERER_KILLS_IPV6 comment (C-1): the flag itself is operator-agnostic and safe to commit, but its companion variables (WANDERER_KILLS_SERVICE_ENABLED, WANDERER_KILLS_BASE_URL) embed the kills app name and differ between .internal/.flycast, so they're set as secrets in Task 6, not here. The reviewer's premise that the flag is "dead" without those companions was overruled by the team lead; only the comment needed fixing. - Add kill_timeout = 30 (I-3): default 5s is tight for this supervision tree on a single-machine full restart. - Add [deploy].strategy = 'rolling' and extend the header comment to name bluegreen/canary as forbidden (I-4): both boot a second machine, which is the same split-brain the header already forbids. - Pin WEB_EXTERNAL_SCHEME = 'http' with a comment on why (I-5): 'https' would rebind off internal_port, force_ssl-redirect the health check forever, and point at /certs files absent from the image (config/runtime.exs:430-444). - Comment on min_machines_running = 1 (I-1): it's inert under auto_stop_machines = 'off' (Fly only applies it under 'stop'/'suspend'); auto_stop_machines = 'off' is what actually delivers always-on. Correcting my prior commit message, which credited min_machines_running with that. - Placeholder comments on `app` (fails loudly if unset) and `primary_region` (fails silently) (I-6). - Rationale comment on [[vm]] sizing (I-7): fifteen Cachex tables and five Finch pools in application.ex (corrected from the review's "sixteen" after counting). Deferred per team lead: I-2 (auto_start_machines) to the human, M-1..M-5 to the final whole-branch review. * fix(fly): flip auto_start_machines to true (I-2, human-approved) Deviates from the brief, which mandates false. Deliberate, human-approved deviation: auto_start_machines = true only starts a machine that already exists — it cannot create a second one, so it does not weaken the single-machine constraint documented at the top of fly.toml. It restores automatic recovery when the machine ends up stopped rather than crashed (interrupted deploy, manual `fly machine stop`, host maintenance); with false the app stays down until a human intervenes. Continues the fix-round-1 series (task-4-review.md) on top of 3200dfe. Not folded into that commit: this worktree's working agreement reserves amending an existing commit for an explicit request from the human user, and this change came from the team lead relaying the human's ruling, not the user directly — a new commit keeps the history reconstructable if that ruling needs to be revisited independently of the other seven items. * fix: restore IPv6 distribution flag for Fly's RELEASE_NODE branch rel/env.sh.eex builds an IPv6 RELEASE_NODE (FLY_PRIVATE_IP is a 6PN IPv6 literal) with RELEASE_DISTRIBUTION="name", but ee15d90 ("fix: removed ipv6 distribution env settings") commented out the paired ERL_AFLAGS="-proto_dist inet6_tcp" in 2025-11, when nothing deployed to Fly yet so the orphaned branch cost nothing. This branch makes the Fly code path live for the first time, and without the flag the release would boot `erl -name wanderer-...@fdaa:...` using the default IPv4 distribution protocol against an IPv6 host — a release_command success followed by an app-machine crashloop at the health check. Move the flag inside the Fly elif branch instead of the top of the file, so docker-compose deployments keep the ee15d90 behavior and only Fly gets IPv6 distribution. * chore: untrack process artifact accidentally force-added to the branch .superpowers/sdd/2026-08-04-flyio-migration/task-2-report.md was force-added in 9dc1a7c by mistake; .superpowers/ is excluded via /app/.git/info/exclude and every other artifact in that directory is correctly untracked. It was 231 of the branch's 731 added lines and contains agent-workflow narration that does not belong in an upstream PR. git rm --cached only: the file stays on disk. * test: actually observe :socket_opts landing in WebSocketClient.start_link/2 The existing suite spent five tests on the pure split_opts/1 helper and asserted only function_exported?/3 for start_link/2 — all six passed if someone swapped the ws_opts/rest arguments at web_socket_client.ex:33-37 or reverted to an inline Keyword.split, which is the exact regression this module exists to prevent. Add module-attribute indirection for :websocket_client (following the existing @pubsub_client / Application.compile_env pattern used elsewhere in this codebase), a Test.WebSocketClientMock via the project's Mox convention, and a test that drives start_link/2 and asserts on the :websocket_client.start_link/4 argument positions directly. Verified this test goes red when ws_opts/rest are swapped and green again after reverting. Also correct the "behaviour conformance" test's comment: it does not catch an upstream callback addition (that's a compile-time @behaviour warning, and there's no --warnings-as-errors gate), only a rename/removal of the two delegated functions. And add Code.ensure_loaded!/1 before the function_exported?/3 assertions to remove a load-order risk under ExUnit's per-seed test shuffling. * docs: fix a wrong line citation and two silent-failure gaps in comments - lib/wanderer_app/kills/client.ex:494 cited websocket_client.erl:276 for the hardcoded 6000ms connect timeout; the correct line is 275. - fly.toml: WEB_APP_URL must be set with an https:// scheme. force_https = true means browsers always arrive over https, so an http:// secret boots cleanly, passes /health, and then fails every LiveView websocket upgrade at check_origin with no automated signal. - fly.toml: note that PROMEX_DISABLED defaults to 'true' (so the missing [[metrics]] block is correct today), and that re-enabling it requires restoring that block; METRICS_PORT defaults to 4021 and nothing currently exposes it. * docs: close the Task 7 decision gate and fix two review nits Task 7 is resolved as Option A -- deploy the kills app from guarzo/wanderer-kills. The gate asked whether Task 5's BIND_IP change had merged upstream in time; it was never actually on the critical path, because Task 8A builds the Dockerfile from a source checkout rather than pulling a published image. Task 8B (Flycast) is struck. Also records the third fork divergence: the nil-telemetry-measurement fix, which is a deploy prerequisite rather than hygiene -- the crash is reachable from any sustained reserved-token burst, which a cold-start backfill produces. The two comment fixes come from the scoped re-review of the final fix round: a line citation that the same commit's own edit had invalidated (replaced with a symbol reference, since line numbers rot), and an ambiguous 'this' in fly.toml that could read as contradicting the paragraph above it. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…per app (#110) An EVE application permits exactly one redirect URL, so the staging period requires a second application rather than merely preferring one. Task 6 step 7 no longer poses this as a question. Records the consequence that was previously left open in Task 11 step 6: because the production hostname does not change when DNS moves to Fly, the production application's redirect URL needs no edit at any point in the migration. The cutover swaps client ID and secret on the Fly app and touches nothing in the EVE developer portal.
On Fly the route builder is reachable only over the IPv6 6PN network.
`route-builder.internal` has no A record, and Mint resolves IPv4-only by
default, so every POST to /route/multiple and /route/findClosest failed
with :nxdomain. Measured from the wanderer machine:
:inet.getaddr(~c"route-builder.internal", :inet) => {:error, :nxdomain}
:inet.getaddr(~c"route-builder.internal", :inet6) => {:ok, {...}}
:gen_tcp.connect(~c"route-builder.internal", 2001, [:inet6], 5000) => {:ok, port}
The symptom did not name DNS. get_all_routes/4 discards the error reason
and falls back to Esi.ApiClient.get_routes_eve/4, whose body is stubbed to
return %{"success" => false} for every hub, so the UI showed "no
connection" on every route with no failure logged.
Mint's `inet6: true` keeps its `inet4: true` default, so it tries IPv6
first and falls back to IPv4 — docker-compose deployments pointing
CUSTOM_ROUTE_BASE_URL at an IPv4 service keep working. That fallback is
why this is unconditional rather than an operator flag: a flag left unset
on Fly fails silently, which is the failure being fixed.
The badge carried an inline `transform: rotate(-90deg)`, which turned the orb icon and count a quarter turn so they read vertically instead of horizontally. It was introduced as `rotate(-90dg)` — an invalid CSS angle unit, so the browser dropped the whole declaration and the badge rendered flat. Fixing the typo to `-90deg` in e791d4e activated a rotation that had never actually rendered, which is when the odd layout appeared. The orb's `marginRight` and the count's `marginTop` are both tuned for horizontal flow, so drop the transform rather than adapt to it. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
fly.toml shipped with `app = '<WANDERER_APP_NAME>'`, a placeholder that no `fly` command accepts. Fly's GitHub integration reads fly.toml from the connected branch to identify the target app, so the placeholder cannot be bound to an app at all — and on the default branch it still finds upstream's 2024 `wanderer-test` file, which is neither this deployment's app nor its region. Name the app the branch actually deploys to. Also route the release command through DIRECT_DATABASE_URL. Ecto's migration lock is a session-scoped Postgres advisory lock and PgBouncer cannot carry session state across transactions, so through the pooled endpoint the lock is taken and lost on a different backend. Nothing fails loudly; concurrent or retried migrations just lose their mutual exclusion. DATABASE_URL stays pooled for the app itself. Trims the comment block throughout, and drops the hostnames that named this particular deployment — the app name is required by Fly's integration and stays, but nothing else here needs to identify a specific install. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…130) `handle_event("toggle-route-alerts", ...)` assigned only `:route_toggle` and never rebuilt `@form`. A checkbox renders `checked` from its form value, so the re-render emitted the box unchecked and LiveView patched the user's tick back out. The hidden fields appeared, which reads as "it worked", and then Save posted `false` and stored route alerts as off — reporting a green "Saved." because with the toggle false the home-system validation has nothing to complain about. The home system itself was persisted; only the flag was lost. Rebuild the form from the submitted params so the box stays checked and anything already typed into `home_system_id` / `route_max_jumps` survives the round-trip. `webhook_url` is forced back to "" first: the generic `.input` writes `value=` for password inputs too, so echoing the params verbatim would print a live webhook URL into the server-rendered HTML. The two existing save tests hand `render_submit` an explicit params map, which bypasses the rendered checkbox entirely — which is why this slipped through. The four added tests drive the DOM instead, supplying params only for fields the user types into.
…ide (#131) `change_settings_tab` assigned whatever tab string the client pushed, so the `:if` guards on the settings tab list were cosmetic: a crafted event rendered the Balance, Subscription or Bots panel even with `map_subscriptions_enabled?` false. Public Api was the only tab with a defence-in-depth body re-check. Gate the handler on an allowlist that mirrors those template guards, keeping the current selection when a tab is unknown or its feature flag is off. This is feature-flag bypass, not privilege escalation. The settings dialog is already gated on the `delete_map` permission in `apply_action(:settings, ...)`, so every actor reaching this handler is a map owner or ACL admin. Also replaces the bare "general" literal with a named attribute.
…ts (#132) The notifications tab needs buttons with distinct visual weights, but button/1 only rendered one style — callers reached for ad-hoc `class="p-button-danger"` to get anything else. Add a `variant` attr mapping onto the PrimeReact severity classes the app already loads, rather than a parallel Tailwind palette that would drift from the React buttons on the map canvas. No new CSS. `:secondary` is the default and composes byte-for-byte the string this component rendered before, so all 47 existing call sites — none of which pass a variant — are unchanged. Callers passing `p-button-danger` via `class=` still compose to the same outlined-danger as today. Note `:primary` emits no modifier on purpose: bare `.p-button` is PrimeReact's filled primary. (`p-button-primary`, passed ad hoc at a few call sites, is defined by neither stylesheet and styles nothing. Those sites are left alone.)
Nobody knows solar system ids, so the home system field is now the same name typeahead the excluded-systems picker uses, single-select, labelled "Jita (The Forge)". Storage is unchanged: the resource still holds an integer solar_system_id, and the route watcher and dispatcher still read it as one. Three things this had to get right: - Option values are strings, not integer ids. LiveSelect re-derives its selection by matching field.value against its options, and a form value round-trips through the browser as a string, so integer values stopped matching the moment the form was rebuilt from params and the chip degraded to a bare id. - The form gained its own phx-change. LiveSelect keeps the selection in its own component state and re-derives it from field.value on every re-render, so any parent re-render with a stale @Form silently blanked the user's pick. The checkbox is unaffected: an input's own phx-change wins over its form's, so toggle-route-alerts still runs alone. - A typed name that resolves to nothing is rejected with a field-level error next to the picker, rather than posting nil and coming back as the resource's generic "is required when route alerts are enabled". Only an exact name is accepted; find_by_name is a substring search, so guessing between "Jita" and "Jitanenba" would watch the wrong system.
…isions (#133) * feat(notifications): resolve Discord channel identity and detect collisions The notifications settings tab rendered a destination as ".../1534657087244603394/40AX••••". That is two problems in one string. The snowflake is the webhook *id* — half of the {id, token} pair that authorises posting — shown in full on a panel that routinely gets screenshotted into support threads. And it identifies nothing: two destinations pointed at the same channel render byte-identical hints, which is how a map ends up with its route alerts quietly sharing the public kill feed. Add ChannelInfo, which resolves a real name in three degrading tiers: 1. GET the webhook URL itself — authorised by the URL alone, returns the webhook name and its channel_id. Always attempted, never carries a bot token it does not need. 2. If a bot token is configured, GET /channels/{id} resolves the real "#channel-name". The bot must share the guild, so a 403 here is the ordinary case for most instances and falls back to tier 1 rather than erroring. 3. A masked hint derived from a truncated SHA-256 of the URL. Unlike the old masked_url/1 the id is masked too, so nothing rendered is recoverable into a credential — and two destinations still differ, because the hash does. colliding_roles/1 reports the role groups that deliver into one channel. This matters most for :route, whose help text tells operators the channel names every system in the chain in order and must be trusted. describe/1 never performs I/O: cache, then the label persisted on the row, then the masked hint, scheduling a background refresh when the cache is cold. The settings template re-renders on every live_select keystroke, and three destinations x two HTTP calls on a render path is a tab that hangs the first time it is opened. resolve/1 is the blocking counterpart for the background refresh and for tests. Resolved identity is cached in :api_cache and persisted on the webhook row so the tab renders a real name on open. Masked results get a much shorter TTL than resolved ones — pinning a placeholder for an hour would freeze a destination after one bad minute. Security notes for review: - channel_id and channel_label are deliberately not sensitive?. The whole point is that they are safe to render on a screen that gets screenshotted, unlike the webhook id shown today. - No cache key or log line carries the webhook URL; everything goes through fingerprint/1. The response body is never inspected either — a proxy error page in front of Discord can echo the request line, and the request line is the URL. - Every HTTP call carries both rescue and catch :exit, matching the defensiveness search_corporations/2 already needed: an unrescued raise on this path killed the whole settings tab on a keystroke. - Writes go through a dedicated cache_channel_info action rather than widening update's accept, so a crafted form submit cannot claim a destination posts to #some-innocent-channel. The settings component is untouched; this lands the interface only. * fix(discord): address review on channel identity resolution Four review findings, all still valid against the current code: - persist/2 discarded the result of cache_channel_info/2, so a rejected write was silent: the row kept its stale label and every refresh retried the same rejected change. It now logs a warning. The summary reports field names and messages only — never inspect/1 on an Ash error, whose InvalidAttribute carries the submitted value and is not redacted by sensitive? true. - resolve_uncached/1 marked a masked fallback as :resolved whenever the webhook named a channel but no name could be read. That cached the hint for the full hour and wrote it over whatever real label the row held. Those results are now :masked, which both persist/2 and ttl_for/1 already treat correctly. channel_id is still carried so collision detection keeps working off the cached id. - HttpClient.get/2 left pool_timeout at Finch's 5_000 default, which on this pool — shared with the far busier delivery path — let an identity read spend @read_timeout on checkout and @read_timeout again reading, double its intended leash. Now 2_000. - Added persistence coverage: the write, the unchanged-value guard, the masked no-op, and the cache_channel_info action itself. That last test exposed a real gap. AshCloak's SetUpEncryption transformer rewrites every create/update/destroy action, removing the cloaked attribute from accept and re-adding it as an action argument. So webhook_url was submittable to cache_channel_info regardless of its accept list, and the redirect the action was split out to prevent was not actually prevented. It is now rejected explicitly. absent/1 does not work here — it falls back to the attribute already on the row and would reject every call — so the validation checks the argument map.
…135) `mix ash.codegen --check` reports pending changes on a clean checkout, and nothing in CI runs that check — every workflow only covers format, compile, credo, dialyzer and tests. The drift has therefore been invisible, and it leaks: the next person to run `mix ash.codegen` for an unrelated resource picks up this diff and either commits it as unexplained noise or spends time working out whether it is theirs. The drift itself is cosmetic. `20260406213852_add_character_description.exs` recorded the `maps_v1.scopes` default as the charlist `'{wormholes}'`, which is how ash_postgres serialized array defaults then; current ash_postgres renders it as `["wormholes"]`. Both are the same `text[]` value, and the type is unchanged, so the migration is a no-op beyond a brief ACCESS EXCLUSIVE lock with no table rewrite. The codegen check is gating here, matching the formatting check in the same job, and runs after the compile step so a real compile error still reports as a compile error. Verified against the dev database: the migration applies (0.0s), rolls back, and re-applies cleanly. `mix ash.codegen --check` and `mix format --check-formatted` both exit 0 on this branch.
… features (#137) The Map Settings → Notifications tab presented three peer webhook destinations and five save buttons, which produced two P0 defects. Route alerts were silently inert. `discord_dispatcher.ex:295` gates delivery on `route_alerts_enabled?`, so a fully configured route channel with the toggle off delivered nothing while the status line read "No kills delivered yet" — the same copy a healthy-but-quiet channel shows. The inverse guard was rendered inside a div that was `hidden` whenever the toggle was off, so the one state needing a warning was the one state where warnings were unreachable. The warning now renders at card level, outside every collapsed container, with an inline control to enable alerts, and route fields are `disabled` rather than `hidden` (review checklist §5). Saves were unscoped. The top form wrote five parent fields while each webhook row wrote a disjoint set, all reporting through one shared flash — so ticking "Enabled" on the System channel and pressing the top Save said "Saved." and reverted the checkbox. There are now two scoped settings forms plus three row forms, each with a panel-local message, dirty-state tracking, and Save disabled until something actually changed. Making disable-don't-hide safe required one load-bearing semantic change: an absent param now means "keep the current value", never "clear it". A disabled input submits nothing, so the previous blanket read would have wiped a saved home system on every save with route alerts off. The tab is reorganised around the two features it actually has — kill notifications and route alerts — rather than three destinations. A map with no configuration sees one URL field and one button; everything else appears once there is something to configure. Disclosures toggle via `JS.toggle_class` following the `characters_live.html.heex:92` precedent, so content stays in the DOM; the server decides the initial state, and a section holding a problem always starts expanded and badges its header. Webhook identity now comes from `ChannelInfo.describe/1` (#133) instead of a mask that rendered the channel snowflake in full and the first four characters of the token. Channel collisions come from `ChannelInfo.colliding_roles/1`, which groups on the resolved channel id and so catches two distinct webhook URLs pointing at one channel — the case that matters, because a route alert names every system between the home system and Jita. Also: Replace is no longer a trap door (it has a Cancel), delivery status is a real state with colour rather than a sentence, the home system is picked by name with a numeric fallback, mentions validate on change rather than only on submit, and the magic numbers behind max-jumps and search limits are named constants.
…136) * zoo(ci): fail the build on duplicate migration module names Two migration files defining the same module is not a compile error. The later definition simply replaces the earlier one, so one migration never runs and the missing schema change only surfaces later as a column or table that was never created. #122 was exactly this: two files both defining `WandererApp.Repo.Migrations.FixMapsScopesDefault`. Nothing in CI would have caught it. `mix ecto.migrate` does reject duplicate *version numbers*, which is why that class of mistake has never made it in, but duplicate module names with distinct timestamps pass every existing check. The check is its own job rather than a step in `static` because it needs neither the BEAM nor deps -- it reports in seconds instead of queuing behind `setup`. It joins the `gate` job's `needs` so it blocks merges through the `Test Suite` context the guarzo/zoo ruleset already requires; a job that is not in that list would go red without stopping anything. Verified both directions: exits 0 on the current tree, and against the pre-#122 tree it exits 1 naming the module and both offending files. `zoo(` prefix because the gate job is fork-specific -- it exists to feed the guarzo/zoo ruleset context, so this diff does not apply upstream. * zoo(chore): drop the dead fleet-readiness flag `WANDERER_FLEET_READINESS_ENABLED` was parsed in `config/runtime.exs` and stored under `:wanderer_app, :fleet_readiness_enabled`, but nothing read it: no `Env` accessor, no call site. An operator who set it to `false` got no effect and no warning, which is worse than having no flag. Deleted rather than wired up, per the decision rule in the fork's own prompt notes -- wire it only with evidence someone intended to gate the feature. There is none: git log -S fleet_readiness --all -- lib/ assets/ # no hits, ever The flag arrived dead in the original `defb2443` mega-commit and no gate was ever written in any branch. Wiring it now at its existing default of `false` would have switched fleet readiness off for every deployment that currently has it on; flipping the default to `true` to avoid that would add a config surface nobody asked for. Removing it is a behaviour no-op precisely because nothing read it. Also updates the "Known gap" callout and the env-var table row in `docs/ZOO-FORK.md`, which both described the flag as unresolved. Verified: `mix compile` generates the app, `mix format --check-formatted` passes on `config/runtime.exs`. * zoo(docs): correct the label system section against the code Audited the Label System section of `docs/ZOO-FORK.md` against `labelIconMap.tsx`, `zooConstants.ts`, `constants.ts`, `labelsManager.ts` and `zoo-theme.scss`. Three things were wrong. **The `crit` and `structure` icons were swapped.** The table gave `crit` a Fire icon and `structure` a Warning icon. The code is the other way round: `crit` is `FaExclamationTriangle` and `structure` is `MdLocalFireDepartment`. The inline comments in `LABEL_ICON_MAP` were already correct -- only the doc had them crossed. **The storage claim was backwards.** The doc said labels are stored using "the original upstream keys (`la`, `lb`, etc.)". They are not. `LABELS` is an enum whose member names are `la`..`l3` but whose *values* are `de`, `gas`, `eol`, `crit`, `structure`, `steve`, and it is the value that `LabelsManager.toggleLabel/1` stores and `toString/0` serializes into `system.labels`. Searching the database for `la` matches nothing. The same false claim was in `labelIconMap.tsx`'s own header comment, which is where the doc copied it from, so both are fixed. **The styles pointer was one file short.** The doc pointed at `constants.ts` / `MARKER_BOOKMARK_BG_STYLES`, but the zoo styles live in `zooConstants.ts` as `ZOO_BOOKMARK_STYLES` and are only spread in there. Worth knowing because that set covers `de`, `gas`, `eol` and `crit` only -- `structure` and `steve` fall through to the upstream `-l2`/`-l3` classes, which is easy to mistake for a missing style. Also documents the `shortName` badges, since those are what actually render on the node: `structure` shows `LP` and `steve` shows `DB`, neither of which matches its menu label. `LP` is "low power" per the source comment; `DB` has no expansion anywhere in the tree and is left as historic rather than guessed at. No behaviour change -- comments and docs only. Verified the four `eve-zoo-effect-color-*` classes referenced do exist in `zoo-theme.scss`, and that `yarn build` and `prettier --check` both pass.
…upstream findings (#138) * zoo(docs): remap prompt-doc SHAs after the history rewrite The force-push that split the three mega-commits changed every SHA above the fork point, orphaning 13 of the 15 commits this document cited. Remapped each to its post-rewrite counterpart by subject, which is sound because the rewrite preserved trees and subjects exactly. The mega-commits have no 1:1 successor, so those citations now name ranges. Their original SHAs stay reachable from origin/guarzo/zoo-prerewrite. Also corrects two claims contradicted by evidence: the 'do not rewrite history' section (overtaken by events, kept with the reasoning), and the advice to delete the backup branches -- each holds 18-38 commits reachable from no surviving ref, so deleting them discards the only copy rather than removing a duplicate. Marks prompts 11, 12, 13 and 15 as landed so a fresh session does not redo them. * zoo(docs): add prompts 16-17 from a fresh post-rewrite upstream review Re-reviewed the 7 commits that landed after this document was written (#129) and had never been assessed for upstreamability. Two are genuine upstream candidates, both verified still-broken upstream rather than assumed: - #131 settings-tab feature-flag bypass. The unguarded change_settings_tab handler is present verbatim on upstream/main and upstream/develop, behind template :if guards that upstream also has -- so the bypass is real there. Written up as feature-flag bypass, not privilege escalation: the dialog is already gated on delete_map, so every actor reaching it is an owner or ACL admin. Overstating it would not survive maintainer review. - #132 button/1 variants. Upstream has no attr :variant. An enhancement rather than a bug, so flagged low priority and best opened as a discussion. The other five are fork-only -- notifications and Discord paths that do not exist upstream -- and are tabulated so nobody re-derives that.
) The `production-deploy` environment's required-reviewer rule has been removed via the API: CI is now trusted as the only gate, so a push to guarzo/zoo whose test suite goes green deploys unattended. The gate had `prevent_self_review: false` with a single reviewer, so in practice it added latency rather than review. The workflow keeps `environment: production-deploy` deliberately. That line no longer gates anything, but FLY_DEPLOY_TOKEN is an environment secret rather than a repository one, so removing it would not fail loudly -- `secrets.FLY_DEPLOY_TOKEN` would expand to the empty string and the deploy would die at `flyctl deploy` with an auth error. The old comment on the deploy step called it a repository secret, which is what would make that cleanup look safe; corrected here and warned about in ZOO-FORK.md. Two invariants change meaning without the human: - The `event == 'push'` clause is now the only guard against a fork PR from a same-named `guarzo/zoo` branch. That used to raise an approval a human could reject; it would now deploy straight to production. - The staleness guard still matters, since `cancel-in-progress: false` queues deploys and a queued run can check out a superseded SHA. Aside from the two "approved for" -> "queued for" strings in the superseded-run notice and job summary, the workflow changes are comments only; the YAML was re-parsed to confirm every functional value is unchanged.
…wn vocabulary (#140) The route alert to Jita was a title, a Path field and an Exit system field. Its stripe was `@color_route = 0x2ECC71` — byte-identical to `@color_kill`. Route alerts share a channel with kill embeds, so the fastest signal in the message said "kill" on a logistics notification. The colour now splits into `@color_route_opened` / `@color_route_improved`: blue, because red, green, yellow and orange are all spoken for by the kill palette, and because blue is The Forge's own colour, which is where the alert always points. The two states are one hue at two lightness steps rather than two hues, so the family reads as "route" at a glance — and since `:improved` carries no ping (`route_ping(:improved, _)` returns nil), the dimmer stripe matches how loud the message is. The embed used three of Discord's slots and left five empty. `author` now carries the state, which frees the title for content: `origin → destination · N jumps`, linked back to the map. That matters because a mobile push preview shows the title and nothing else. The origin resolves map-local first, so a map naming its home "Home" reads as "Home → Jita" with no special-casing in the formatter — that naming decision belongs to the map. The path moves into the description with the destination bolded, and a `timestamp` renders client-side as local time, because a qualifying route is perishable and "how old is this" is part of the decision. An `:improved` alert said "3 jumps". 3 → 2 is a shrug and 7 → 2 is news, and the bare total could not tell them apart. The transition table in `RouteWatcher` is the only place that still knows the previous count, so `previous_jumps` is threaded from there through `alert/6` into `deliver_alert/8`. It is additive: an improved alert without it renders the plain total rather than raising inside the formatter, where the failure would cost the whole alert. The Exit field was unconditional. `find_exit_system/2` returns the first non-wormhole system in path order, so a chain popping straight into Jita returned Jita and the embed rendered "Exit system: Jita" directly beneath a path ending in Jita. It now appears only when the exit is somewhere else — where it is the most decision-relevant fact in the message — and carries the gate distance that makes it actionable. What the alert guarantees was invisible. `Evaluator`'s pinned `@solver_settings` are the whole value of the message: the route needs no scouting. A footer now states them in the reader's language. It deliberately makes no ship-class claim — `include_cruise: true` means a cruiser-sized hole qualifies, so "freighter- safe" would be false, and the evaluator's own comment says "hauler". The three exclusions are stated plainly and the reader judges their own hull. `route_guarantee_settings/0` and its test pin the string to the settings it describes, because drift here turns a safety guarantee into a lie that reads exactly as authoritative as it did when true. `map_url/1` returns nil rather than a best-effort URL on every failure path. A malformed `url` is a 400 from Discord, which is a delivery failure, which counts toward `@max_consecutive_failures` and can auto-disable the destination. `Env.base_url/0` defaults to the literal placeholder "<BASE_URL>" when unconfigured, so the scheme check is load-bearing, not defensive padding. The path now lives in the description, bounded by 4096 rather than a field's 1024 — the truncation test moves to 250-character names accordingly, since the old 60-character names no longer breach the looser bound.
…b in two (#141) The Notifications tab had three problems that all came back to the same thing: it showed what it had stored rather than what it knew. **Channel identity.** A destination read "Channel: Zoo Killfeed" whether that was a real `#channel-name` or just the nickname whoever created the webhook typed into Discord's dialog. `ChannelInfo` collapsed both tiers into `source: :resolved`, so the UI could not tell them apart and asserted the stronger one. `channel_label_source` now persists which tier answered, and the row says "Channel: #kill-feed" or "Webhook: Zoo Killfeed" accordingly — the second with a line explaining that Discord only names the channel when this instance's bot is in that server. A label written before the column existed reports `:unknown` and renders bare: guessing the tier from a leading "#" is exactly the inference the column exists to stop, and a webhook may legitimately be named "#anything". Those rows self-heal on the next refresh; there is no backfill because nothing recorded enough to write one. **Mentions.** Mention targets were a single CSV field where an operator typed `user:1234,role:5678` by hand. Discord renders an unknown mention as inert text — no error, just a message that pings nobody — so a typo failed silently and permanently. There are now separate Users and Roles pickers backed by `Discord.Guild`, which reads `GET /guilds/{id}/roles` and `GET /guilds/{id}/members/search` against the guild this destination actually posts to. That guild is per-webhook, resolved by `ChannelInfo` from the channel read; the installation-wide `DISCORD_GUILD_ID` could not serve this, because a map pointed at a different guild would be offered ids that are inert there. `@everyone` is excluded from the role list — it is the one entry that wakes a whole server, and one click from a kill-feed toggle is not a picker. Both reads need a bot token and the bot to be in the guild, neither guaranteed. `no_bot_token`, `unauthorized` and `forbidden` are ordinary outcomes, not faults: the picker swaps for a manual add-by-id input that says which of the two is missing. They stay distinct because "this instance has no bot" and "your guild has not invited ours" have different fixes. **Layout.** The tab nested route alerts inside a kill-notifications box, which put chain-topology settings under a heading about kills and made the kill-scoped filters look like they applied to both. They are now two peer cards. The filters disclosure stays collapsed and badges its problem rather than springing open, so a message can no longer render inside a closed body where nobody sees it — the tests assert placement, not just presence. Also: the route row no longer says "No kills delivered yet" under a channel that only ever carries route alerts; chips use the house `badge badge-ghost` idiom; a masked route channel now re-renders when the async identity refresh lands, via a `notify:` option on `describe/2`. The refresh message is a three-tuple deliberately. `MapsLive` carries an unguarded `handle_info({ref, result}, socket)` catch-all that calls `Process.demonitor(ref, [:flush])`, which raises on a non-reference — a two-tuple would take the whole map LiveView down on the first refresh. Ordering a clause above the catch-all would also work and is worse: it makes correctness depend on a source position any later edit can silently break. Security notes carried forward: no path inspects an Ash error struct (`Ash.Error.Invalid` carries the submitted webhook URL in `value:` and `sensitive? true` does not redact it), nothing logs a bot token or a response body, and the write boundary on the new cached fields is the update action's restricted `accept` list, not the cache action's validation — pinned by a test. Verified: full suite 1711 tests / 0 failures, `mix format --check-formatted`, `mix credo --strict` clean on touched code, `mix compile --warnings-as-errors`. Not verified: `members/search` against a live guild (no bot token here), so whether it needs the privileged GUILD_MEMBERS intent is untested — a 403 there degrades into the manual fallback, so the failure mode is contained.
…or (#143) `jest.config.js` sets `testEnvironment: 'jsdom'`, but Jest 28 stopped shipping that environment and it was never added as a dependency, so `yarn test` failed to start at all — no suite ran, and the failure looked like a config typo rather than a missing package. Adding it at the version matching the installed Jest (29.7.0) makes `yarn test` work as written, and leaves the door open for component tests that actually need a DOM. `SolarSystemNodeZoo.tsx` also carried a lone `prettier/prettier` error, so `eslint` on that file failed regardless of what you changed there. Repo-wide lint is still red — 356 errors and 61 warnings across 99 of 498 files, 275 of them formatting. That is a separate cleanup, deliberately not bundled here.
… 12h (#142) * fix(zoo): show scan age for unscanned signatures, stop hiding it past 12h The age bookmark on the zoo node filtered signatures with `group === 'Wormhole' && !linked_system` — the same predicate as `useUnsplashedSignatures`, which answers a different question: "which wormholes are still unmapped". Signatures pasted straight from the probe scanner carry `group: 'Cosmic Signature'` until a type resolves, so a freshly scanned system matched nothing and showed no age at all until at least one signature turned out to be a wormhole. Every signature now counts. Pasting re-stamps every signature in the window — untouched rows are still sent as updates by `getActualSigs` — so the newest timestamp in a system is the time of the last paste, which is exactly what the indicator claims to show. The 12h cliff in `getBookmarkColor` was overloading the same -1 sentinel to mean "too old to display", making a long-neglected system render identically to one nobody has ever scanned. Ages now render without an upper bound, in a fourth colour band, and switch from hours to days at 24h so the bookmark keeps its width. A missing bookmark now means one thing only: never scanned. The age math moves to `helpers/signatureAge.ts` as pure functions so it can be tested without a React renderer. * fix(zoo): keep the inserted_at fallback when updated_at will not parse `new Date('garbage').getTime()` is NaN, and NaN loses every `>` comparison, so a signature with an unparseable `updated_at` did not merely fail to contribute its own timestamp — the early return meant its `inserted_at` was never consulted at all, and the signature collapsed to 0 exactly as if it carried no timestamp. A system whose only signature had a malformed `updated_at` showed no scan age despite having a perfectly good `inserted_at`. Parsing now rejects non-finite results at the point of parse, so the fallback chain behaves the same for malformed values as it does for missing ones. Raised in review on #142.
…licates after restart (#144) * docs(spec): design for Discord killmail notification fixes Two production defects, fixed independently: 1. Kills fire for systems removed from a map. SystemMapIndex builds its fan-out index with get_all_by_map/1, which has no visible filter, and removal is a soft delete. Fixed by switching to get_visible_by_map/1, plus a fail-open membership guard in the dispatcher to bound the index's five-minute staleness window. 2. Duplicate posts after a restart. The dedup marks live in an in-memory Cachex, and the 3600s freshness default admits an hour of upstream replay once they are gone. Fixed with a tighter maximum age during a grace window armed from a sentinel in the dedup cache itself, so the window tracks that cache's lifecycle rather than the dispatcher's. Incorporates four findings from an independent Codex review. * docs(discord): implementation plan for the killmail notification fixes Five TDD tasks: filter the fan-out index by system visibility, a fail-open map-membership guard in the dispatcher, configuration for the startup window, the window itself armed from a dedup-cache sentinel, and drop telemetry. Three deviations from the spec, each found by re-reading the code rather than the prose: - The telemetry prefix is :discord_dispatcher, not :discord. In this module :discord belongs to the enrichment events; a drop is a dispatch outcome and shares the shape of :dispatched and :not_delivered. - The sentinel needs Cachex.put/3 AND Cachex.persist/2. The dedup cache has a 24h default_ttl and Cachex honours only an integer :ttl, so the spec's 'no TTL' was not expressible in one call -- and without persist the fix would have decayed invisibly after a day of uptime. - The unfiltered index query is at system_map_index.ex:103, not :98. Also corrected while writing: an 'if' cannot be a with-clause in either its block or keyword form, so the startup max-age binding is hoisted above the chain. Verified against elixir 1.17.3, along with every embedded code block's formatting. * docs(discord): revise the killmail plan after independent review Five defects found by reviewing the plan against the code. The sentinel is now read once per kill batch instead of once in init/1. Caching the deadline in dispatcher state reintroduced the dispatcher's lifecycle through the back door: the dedup cache and the dispatcher are independent children of a one_for_one supervisor, so a cache-only crash never runs init/1 and a long-lived dispatcher would hold a stale deadline while every mark was gone -- the exact case the sentinel design was chosen to handle. Reading per batch is also simpler: no state field, no init/1 change, and do_dispatch/2 keeps its arity across all three clauses. Both new config keys are wired into config/runtime.exs and documented in .env.example and README.md. Accessors alone left them pinned to their defaults in every release. Age drops are classified per kill against both thresholds rather than by whether the window is open, so a kill the pre-existing hour limit would have dropped anyway is no longer counted as new suppression. Tests that could pass for the wrong reason are tightened: deadline comparisons vary the grace tenfold instead of relying on millisecond resolution, and delivery assertions require an actual HTTP request rather than only the absence of drop telemetry. The Cachex claim is cited to the Hex package rather than a deps/ path that does not resolve from a worktree. * fix(kills): index only visible systems for kill fan-out Removing a system from a map is a soft delete, but SystemMapIndex built its system->maps index with get_all_by_map/1, which has no visible filter. Every system a map had ever contained mapped to that map permanently, so kills kept broadcasting for removed systems -- to the in-app kills widget and to Discord. Affects the kills widget as well as Discord, in the same direction: a system that was removed should not light up with kill activity. * fix(discord): drop kills for systems no longer on the map Task 1 fixes persistent index membership but not staleness: the index refreshes only after a successful kills-client subscription update, and otherwise on a 5-minute timer, so a removed system keeps producing kill broadcasts for up to five minutes. This guard consults the live map cache, which remove_system/2 updates immediately. It is fail-open -- it drops a batch only when the map cache reads successfully AND positively lacks the system. An unreadable or absent cache entry lets the batch through, because a map with no live GenServer is not evidence that a system was removed. Placed before the age filter and dedup, so a dropped kill is never marked attempted and stays eligible on a later arrival. * feat(discord): config for the killmail startup grace window Two keys, deliberately not sharing a validator. discord_startup_grace_seconds (default 600) is non-negative: 0 is a legitimate 'no startup window', and the positive-integer validator would turn that into 600 seconds plus a warning. discord_startup_max_killmail_age_seconds (default 120) keeps the positive-integer validator, for the same reason the ordinary max age does: 0 would silently suppress every notification. Both are wired through config/runtime.exs, which is the only config file a release reads -- an Env accessor alone would leave them pinned to their defaults in production. config/test.exs sets the grace to 0 so existing dispatcher tests are not silently pulled inside a live window. * fix(discord): suppress replayed killmails after a restart The dedup marks live in an in-memory Cachex, so a restart loses every one. The kills client then rejoins its channel, the upstream service replays recent killmails, and the ordinary 3600-second freshness limit admits an hour of already-posted kills. For a grace window after the marks are lost, the freshness filter uses a much tighter maximum age instead. Replayed history is dropped because it is old; a killmail that genuinely occurs during the window still posts. The window is derived from a sentinel in the dedup cache, not from the dispatcher's own lifecycle, and it is read once per kill batch rather than at dispatcher start. The cache and the dispatcher are separate children of a one_for_one supervisor: a dedup-cache-only crash loses every mark while the dispatcher keeps running, so anything keyed to the dispatcher's lifecycle -- including a deadline cached in its state at init -- goes stale in exactly that case. The sentinel holds an absolute monotonic deadline rather than a TTL, so an expired window is still a present sentinel and nothing re-arms it while the cache lives. * feat(discord): report why a killmail was not posted A killmail dropped for age or as a duplicate left no trace at all: the filters fall out of the with chain into a catch-all :ok, and telemetry fired only after delivery or an enqueue failure. That made 'did we suppress it, or did we never receive it?' unanswerable -- the one question the new startup window makes worth asking. Emits [:wanderer_app, :discord_dispatcher, :killmail_dropped] with %{count: n} and a reason of :startup_age, :age, or :duplicate. Three reasons rather than one, because conflating the new suppression with the pre-existing hour limit would defeat the point. Silent when nothing was dropped. The two age reasons are classified PER KILL against both thresholds, not by whether the window is open. Inside the window a kill old enough to fail the pre-existing hour limit would have been dropped anyway, and reporting it as :startup_age would inflate the window's apparent impact in the one metric used to judge whether the window is too aggressive. Prefix is :discord_dispatcher, matching :dispatched and :not_delivered -- a drop is a dispatch outcome. The :discord prefix in this module belongs to the enrichment events. One throttled Logger.info per batch for :startup_age only. * fix(discord): restore gate ordering, distrust half-built map cache, trace :not_on_map drops Three findings from the whole-branch review: 1. Age config resolution and startup-window arming were plain bindings above the `with` chain, so every `:map_kill` batch on every map paid a Cachex read, two Env reads, and any misconfiguration warning even with Discord disabled globally or for that map. Extracted into `age_limits/0` and called as a `with` clause below the enablement gates; still resolved once per batch, never per kill. 2. `WandererApp.Map.new/1` commits the cache entry with `systems: %{}` and fills it one `update_map/2` at a time, so during map start the entry reads as a positive "not on this map" for every system and kills arriving in that window were dropped and lost. `system_on_map?/2` now requires the `map_\#{id}:started` flag `can_broadcast?/1` uses, and fails open without it. 3. The membership guard was the one new drop path with no telemetry. Added a fourth `:killmail_dropped` reason, `:not_on_map`, emitted batch-level.
…#145) * feat(notifications): rebuild the settings tab around what is true now The Notifications tab did not fit on a laptop. Every section was expanded by default inside a dialog with no height of its own, so the lower half was unreachable, and the tab carried five separate Save buttons. Reorganised around the two features the schema actually models — kill notifications and route alerts — with each destination shown as a truth line and every control that changes it behind an edit. Kill switches apply on change; route alerts keep one Save because their three fields are validated against each other. No global controls remain. Route alerts no longer require a kill webhook first: `create`'s `webhook_url` is optional and the policy row is created lazily by whichever control the operator touches first. All three destinations are now removable. Also fixes three things the work surfaced: the settings dialog could not be scrolled to reach a tall panel, the tab strip's ARIA was hand-transcribed and wrong, and `live_select`'s `:label` was accepted but never rendered. * fix(notifications): removable kill channel crashed; rejected URL poisoned retry Two defects CodeRabbit caught in the previous commit, both introduced by widening `remove-webhook` to `:system` and by the lazy policy-row create. `role_label/1` had no `:system` clause. Widening the handler without widening the label raised FunctionClauseError *after* the destroy had committed, so removing the kill channel took the tab down and left the destination gone. Every role now has a clause, and the strings are the row titles verbatim, so the confirmation names the row the operator clicked. `save-webhook` left `@notification` nil when `ensure_notification/1` committed the policy row and the webhook write then failed. The correction resubmitted into a second `create` for the same map, which `unique_map_id` rejects — so a rejected URL poisoned every retry with "has already been taken". Only the one key is assigned on that path; the full cascade would rebuild `webhook_forms` and `replacing_url?` and discard the URL being fixed. The gap that hid the first one was a test asserting the Remove button exists without ever clicking it. Both defects now have regression tests, each confirmed to fail against the unfixed code.
…he defect (#146) * fix(tracker): restore location tracking on map re-entry, instrument the defect A character could sit on a map with tracked=true, online, green indicator, correct system — and simply stop moving. Recovery required an EVE relog. `update_location/1` only matches `%{track_location: true, is_online: true}`; anything else falls through to a catch-all returning `{:error, :skipped}` with no log, telemetry, or exception. In production five characters were frozen with `is_online: true, active_maps: 1, track_location: false`. Two writers set `track_location`, and both miss this case: - `maybe_start_location_tracking/2` matched on an explicit `track_location` key in the incoming settings. No caller anywhere passes it — not `TrackingUtils.track_character/4`, not either re-track path in the map server. The clause was dead code. - `update_online/1` sets it, but only inside a `online != is_online` transition gate. A fresh tracker defaults to `is_online: false`, so the first ESI poll is always a transition — which is why this normally works. That leaves one unreachable-by-repair state: `is_online: true` with `track_location: false`. `maybe_stop_tracking/2` is its only producer, clearing the flag when `active_maps` empties while leaving `is_online` alone. Presence expiry (a browser websocket drop past the 15m grace period, not an EVE logout) triggers it. If the tracker is GC'd first the state is rebuilt clean; return inside that window and it is frozen permanently. Derive location/ship tracking from `active_maps` instead, making the "on" path symmetric with `maybe_stop_tracking/2`'s "off" path. The user's selection still governs entirely: `active_maps` is populated only from `track:`, and an empty list cannot match `[_ | _]`. Adds three counters to validate the mechanism in production: - `location_flag_cleared` — the bad pair is created - `location_flag_repaired` — a character who would have frozen was restored - `location_skipped_while_active` — a character is frozen now; must be zero `repaired > 0` with `skipped == 0` confirms both the diagnosis and the fix. Registered as base metrics so WANDERER_BASE_METRICS_ONLY cannot hide them. Also allowlists the tracking metadata keys in the Logger config. character_id, map_id, tracking_pool and friends were annotated on error logs but silently discarded, which is why an earlier round of "add logging to troubleshoot this" produced nothing usable. Does not repair already-stuck characters: `reconcile_tracking/1` only restores the tracking_start_time cache key and never calls update_track_settings. They recover on re-track or tracker GC. * fix(tracker): count location_flag_cleared transitions, not untrack calls CodeRabbit review on #146: the telemetry guard checked only is_online, so a repeat untrack for a character whose track_location was already false still incremented the counter. That inflates :location_flag_cleared against the :location_flag_repaired counter it exists to be read against, which is the whole point of the instrumentation. * test(tracker): assert the skip counter is throttled, not just emitted CodeRabbit follow-up on #146. The throttle test covered the log line but not the telemetry, so moving :telemetry.execute outside the Cache.put_new guard would have passed while turning the counter into a per-tick measure. Also records why :location_skipped_while_active has no legacy-stuck drain period to discount: the cache is in-memory, so the deploy clears it.
…147) * chore(metrics): enable PromEx so the tracking defect counters record #146 added :location_flag_cleared, :location_flag_repaired and :location_skipped_while_active, but PROMEX_DISABLED defaults to 'true' and fly.toml had no [[metrics]] block, so the counters were inert. Two of the three have no companion log line, including the origin event, so the freeze could not be correlated against its trigger. Binds the metrics server to IPv6 any rather than 0.0.0.0: Fly scrapes over the IPv6-only 6PN network, and bound to IPv4 the endpoint answers a local curl while the scrape silently returns nothing. * chore(metrics): fail boot when METRICS_PORT diverges from Fly's scrape port CodeRabbit review on #147. fly.toml's [[metrics]] block hardcodes 4021 and cannot read METRICS_PORT, so setting the env var to anything else means PromEx serves /metrics on one port while the scraper polls another. That presents as 'the app emits no metrics' rather than as a misconfiguration, which is the same silent failure the IPv6 bind comment guards against. Scoped to Fly via the existing app_name != NOT_FLY_APP idiom, so self-hosted deployments can still choose any port. * chore(metrics): bind IPv4 off Fly, test the port literals against drift CodeRabbit review on #147, two findings. The unconditional IPv6 bind was a regression for self-hosted deployments: binding :: on a host booted with ipv6.disable=1 fails with :eafnosupport, which takes the whole app down rather than just the metrics server. Fly still gets IPv6 because its 6PN scrape network is IPv6-only; everyone else gets the 0.0.0.0 they had before. The boot guard added in c8c0c50 only caught METRICS_PORT drifting away from the 4021 literal. It could not catch fly.toml drifting, because by then the literal is the stale side. A test now asserts all three copies of the port agree; it runs under the existing mix test CI job, so no new workflow.
`untrack_characters/2` hardcoded `reason: :presence_expired` on the
`:stopped` event. Three different branch points reach it — presence
departure, ACL revocation, and the admin Untrack button — and all three
were reported as presence expiry. A constant wearing a variable's name:
the label existed, so the metric looked dimensioned, and every query
grouped by it got one answer.
The reason is now an argument, bounded to
`:presence_driven | :acl_revoked | :manual_untrack` by a guard so a
typo raises instead of quietly minting a new Prometheus series.
Threading it through the untrack path needed one non-obvious step. The
presence path does not call the tracker directly; it queues
`{map_id, character_id}` for a drain five minutes later, and that tuple
is the queue's uniqueness key under `Enum.uniq_by`. Widening it would
have broken every reader, so the reason rides in a sidecar cache key
that the drain reads and deletes. A repeat untrack of the same pair
overwrites it, which is the behaviour we want.
The tracker keeps the reason as `last_cleared_reason` and reports it on
`location_flag_repaired`, so the repair counter says what it repaired.
That distinction matters for reading the metric:
`repaired{reason=presence_driven}` is the fix saving a character who
would have frozen on the map, while `repaired{reason=manual_untrack}`
is the fix reverting an operator who deliberately pressed Untrack.
Only the first belongs in the headline number. The field is held in
`:character_state_cache`, which is in-memory, so a clear that predates
a restart reports `:unknown` rather than guessing.
Also registers `[:wanderer_app, :character, :tracking, :stopped]`,
which has been emitted on every untrack since long before the plugin
existed and collected nowhere. It is the denominator the other two
counters are read against: cleared/stopped is the share of untracks
that caught a character still online in EVE.
The handler called `Map.Server.untrack_characters/2` and nothing else.
That stops the tracker, but the `map_character_settings` row still said
`tracked: true`, so every reader that consults settings — including
this page's own button, which stayed lit — reported the character as
tracked. The operator pressed Untrack, got a success flash, and saw no
change anywhere they could look.
Two halves to the fix. The settings row is now written, and the
rendered `tracked` is derived from those rows rather than from the
presence cache the character arrived in. The second half is not
cosmetic: this page untracks *other users'* characters, and it cannot
write another user's presence entry, so reading presence here could
never reflect the action it had just taken.
The tracker call runs whether or not the settings write succeeds. A
failed write leaves the row stale, which is exactly the state the
previous code produced unconditionally, but it must not also skip the
untrack.
This does not make the untrack durable, and the handler now says so.
`track_character/2` deliberately re-tracks a character whose settings
say `tracked: false` when they next enter presence with valid tokens
and permissions, so an untracked character returns on their next map
entry by design. Worth knowing before reading the metrics: this means
`repaired{reason=manual_untrack}` will keep firing after this change,
and a drop to zero would indicate auto-restore had broken.
Both files here are byte-identical to upstream/main, so this is an
inherited defect rather than fork drift, and a candidate for a separate
upstream PR.
Three defects in this investigation shared a shape: source that states
an intent, a runtime that discards it, and nothing anywhere that
disagrees out loud. These two tests catch that shape statically.
Telemetry, both directions:
* `[:wanderer_app, :esi, :error]` had a PromEx counter declared and
no `:telemetry.execute/3` anywhere in `lib/`. The series never
materializes, so a panel reading it shows "no data" — visually
identical to "ESI is healthy", and an alert on it can never fire.
Now emitted from the six failure branches in `api_client.ex`, with
numeric ids stripped from the endpoint label so the path does not
open one series per character.
* 86 events are emitted and registered nowhere, paying the emission
cost for data that is discarded. Registering all of them is out of
scope, so 85 are recorded as a baseline the test holds the line
against — nothing new may join without a deliberate decision, and
a third test fails if an entry becomes stale. The 86th,
`:character, :tracking, :stopped`, is registered in the first
commit of this branch.
Cachex: every cache in `application.ex` was declared with
`default_ttl:`, which is a Cachex 2.x option. This project runs 3.6,
which ignores options it does not recognise rather than rejecting
them, so fifteen caches claimed an expiry policy and applied none.
Two comments elsewhere had already been written reasoning from those
TTLs as real; both are corrected here.
The options are removed rather than translated. Turning real
expiration on across fifteen caches at once is a production behaviour
change that deserves a per-cache decision — `:character_state_cache`
at 1h could evict an idle character's state and produce a freeze
indistinguishable from the bug this branch is chasing. The intended
TTLs are recorded in a comment so restoring one is a decision and not
an archaeology exercise.
Both tests read source rather than starting the app: a dead option is
dead whether or not the cache is started, and an event only reachable
under rare runtime conditions still has to be declared in both places.
📝 WalkthroughWalkthroughThe change removes unsupported Cachex TTL options, propagates untracking reasons through map and tracker flows, adds reason-tagged tracking metrics, emits telemetry for ESI GET failures, and adds source-based consistency tests. ChangesTracking observability and cache configuration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant MapCharactersLive
participant MapServerCharactersImpl
participant TrackerManagerImpl
participant Tracker
participant PromEx
MapCharactersLive->>MapServerCharactersImpl: request manual untracking
MapServerCharactersImpl->>TrackerManagerImpl: queue reason :manual_untrack
TrackerManagerImpl->>Tracker: apply untrack_reason
Tracker->>PromEx: emit tracking stop with reason
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
🧪 Test Results Summary
Full output for the advisory checks is attached to this run as 🔧 Reproduce locallymix format
mix test
mix credo --strict
mix dialyzer🤖 Auto-generated by GitHub Actions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/wanderer_app/character/tracker_manager_impl.ex`:
- Around line 193-199: Update remove_from_untrack_queue/2 to delete
untrack_reason_key(map_id, character_id) whenever it removes the corresponding
queue entry, ensuring cancelled entries and stale reasons are cleared from
Cachex.
- Around line 390-391: Update the delayed-untrack telemetry event to use the
retrieved reason variable instead of the hardcoded :presence_left value.
Preserve the existing reason propagation through the tracker call and ensure
both manual and ACL-driven delayed untracks emit the actual reason.
In `@lib/wanderer_app/character/tracker.ex`:
- Around line 1311-1318: Update the untrack_reason/1 clauses to return the input
only for the supported reasons :presence_driven, :acl_revoked, :manual_untrack,
and :unknown; map nil, non-atoms, and all other atoms to :unknown. Preserve the
existing fallback behavior for missing untrack_reason values.
In `@lib/wanderer_app/esi/api_client.ex`:
- Around line 519-524: Update endpoint_label/1 to remove the query string before
path normalization, then replace each complete non-empty path segment that
represents an identifier—including alphanumeric killmail hashes—with the shared
:id placeholder. Ensure matching is segment-anchored so values such as /2abc are
normalized as a whole segment rather than partially rewritten.
In `@test/unit/telemetry_registration_test.exs`:
- Around line 136-164: Update emitted_events/0 to parse telemetry calls through
the AST rather than only regex-matching literal lists, resolving module
attributes whose values are literal event lists. Use AST analysis to retain atom
literals, but reject an entire event list when it contains runtime values such
as map_id instead of filtering individual tokens. Preserve the existing
wanderer_app filtering and MapSet collection behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7cda300-302c-40c1-b9ca-2c937dfd5225
📒 Files selected for processing (13)
lib/wanderer_app/application.exlib/wanderer_app/character/tracker.exlib/wanderer_app/character/tracker_manager_impl.exlib/wanderer_app/esi/api_client.exlib/wanderer_app/external_events/discord_dispatcher.exlib/wanderer_app/map/map_server.exlib/wanderer_app/map/server/map_server_characters_impl.exlib/wanderer_app/map/server/map_server_impl.exlib/wanderer_app/metrics/prom_ex_plugin.exlib/wanderer_app_web/live/map/map_characters_live.extest/unit/cache_options_test.exstest/unit/external_events/discord_startup_window_test.exstest/unit/telemetry_registration_test.exs
Comment on lines
+519
to
+524
| defp endpoint_label(path) when is_binary(path) do | ||
| path | ||
| |> String.replace(~r{/\d+}, "/:id") | ||
| |> String.split("?") | ||
| |> List.first() | ||
| end |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
endpoint_label/1 still lets unbounded values into the endpoint label.
The regex replaces only digit runs. get_killmail/3 builds "/killmails/#{killmail_id}/#{killmail_hash}/" (Line 149). The hash is a 40-character hex string and is not purely numeric, so it survives normalization and produces one Prometheus series per killmail. That is the exact cardinality problem the comment above says the function prevents.
Two smaller points on the same function:
~r{/\d+}is not anchored to a full segment, so/2abcbecomes/:idabc.- The query string is removed after the replacement, so digits inside a query string are rewritten first.
Normalize whole segments and strip the query string first.
♻️ Proposed fix for segment normalization
defp endpoint_label(path) when is_binary(path) do
path
- |> String.replace(~r{/\d+}, "/:id")
|> String.split("?")
|> List.first()
+ |> String.split("/")
+ |> Enum.map_join("/", fn
+ "" -> ""
+ segment -> if opaque_segment?(segment), do: ":id", else: segment
+ end)
end
defp endpoint_label(_path), do: "unknown"
+
+ # Numeric ids and hex hashes (killmail hashes) are per-request values and must
+ # not reach a Prometheus label.
+ defp opaque_segment?(segment) do
+ Regex.match?(~r/^\d+$/, segment) or Regex.match?(~r/^[0-9a-fA-F]{16,}$/, segment)
+ end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/wanderer_app/esi/api_client.ex` around lines 519 - 524, Update
endpoint_label/1 to remove the query string before path normalization, then
replace each complete non-empty path segment that represents an
identifier—including alphanumeric killmail hashes—with the shared :id
placeholder. Ensure matching is segment-anchored so values such as /2abc are
normalized as a whole segment rather than partially rewritten.
Comment on lines
+136
to
+164
| defp parse_event_list(raw) do | ||
| raw | ||
| |> String.split(",") | ||
| |> Enum.map(&String.trim/1) | ||
| |> Enum.reject(&(&1 == "")) | ||
| |> Enum.map(&String.trim_leading(&1, ":")) | ||
| |> Enum.map(&String.to_atom/1) | ||
| end | ||
|
|
||
| defp registered_events do | ||
| plugin = File.read!(@plugin_path) | ||
|
|
||
| ~r/@[a-z_]+_event\s+\[([^\]]*)\]/ | ||
| |> Regex.scan(plugin, capture: :all_but_first) | ||
| |> Enum.map(fn [raw] -> parse_event_list(raw) end) | ||
| |> Enum.filter(&match?([:wanderer_app | _], &1)) | ||
| |> MapSet.new() | ||
| end | ||
|
|
||
| defp emitted_events do | ||
| source_files() | ||
| |> Enum.flat_map(fn path -> | ||
| ~r/:telemetry\.execute\(\s*\[([^\]]*)\]/ | ||
| |> Regex.scan(File.read!(path), capture: :all_but_first) | ||
| |> Enum.map(fn [raw] -> parse_event_list(raw) end) | ||
| end) | ||
| |> Enum.filter(&match?([:wanderer_app | _], &1)) | ||
| |> MapSet.new() | ||
| end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find telemetry emitters that do not use a literal event list.
set -euo pipefail
# Attribute- or variable-based event names.
rg -nP ':telemetry\.execute\(\s*[`@a-z_`]' lib
# Event lists containing non-atom (dynamic) segments.
rg -nPU ':telemetry\.execute\(\s*\[[^\]]*\]' -o lib | rg -nP '\[[^\]]*(?<![:\w])[a-z_]+\w*\s*[,\]]'Repository: guarzo/wanderer
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f 'telemetry_registration_test\.exs$|\.exs$' . | head -200
printf '%s\n' '--- relevant symbols and telemetry calls ---'
rg -n -C 4 'defp (parse_event_list|registered_events|emitted_events)|:telemetry\.execute|@[a-z_]+_event' . -g '*.ex' -g '*.exs' || trueRepository: guarzo/wanderer
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- telemetry registration test ---'
sed -n '1,190p' test/unit/telemetry_registration_test.exs
printf '%s\n' '--- registration declarations in the plugin ---'
rg -n -C 3 '@[a-z_]+_event\s+\[' lib test config || true
printf '%s\n' '--- all execute call heads in lib ---'
rg -n ':telemetry\.execute\(' lib | sed -n '1,240p'Repository: guarzo/wanderer
Length of output: 25555
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- remaining assertions ---'
sed -n '185,260p' test/unit/telemetry_registration_test.exs
printf '%s\n' '--- plugin event attributes and uses ---'
sed -n '1,180p' lib/wanderer_app/metrics/prom_ex_plugin.ex
rg -n -P '@[a-z_]+_event|:telemetry\.execute\(\s*@[a-z_]+_event|:telemetry\.execute\(\s*[a-z_][a-z_0-9]*\s*,' lib
printf '%s\n' '--- exact scanner results and non-literal list heads ---'
python3 - <<'PY'
from pathlib import Path
import re
execute = re.compile(r':telemetry\.execute\(\s*\[([^\]]*)\]')
dynamic = re.compile(r':telemetry\.execute\(\s*([^,\[\s][^,()]*)')
for path in sorted(Path('lib').rglob('*.ex')):
text = path.read_text()
for m in execute.finditer(text):
raw = m.group(1)
tokens = [x.strip() for x in raw.split(',') if x.strip()]
if any(not x.startswith(':') for x in tokens):
print('dynamic-segment', path, repr(raw))
for m in dynamic.finditer(text):
arg = m.group(1).strip()
if arg.startswith('@') or re.fullmatch(r'[a-z_][a-z_0-9]*', arg):
print('non-list-event-argument', path, repr(arg))
PYRepository: guarzo/wanderer
Length of output: 22657
Make the telemetry source scan handle non-literal event lists.
- If an emitter passes a literal module attribute,
emitted_events/0misses it and can report a false failure. Resolve module attributes that contain literal event lists. - If a list contains
map_id, reject the entire list. Filtering only dynamic tokens creates a truncated event path and can hide mismatches. Use AST parsing to distinguish atom literals from runtime values.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unit/telemetry_registration_test.exs` around lines 136 - 164, Update
emitted_events/0 to parse telemetry calls through the AST rather than only
regex-matching literal lists, resolving module attributes whose values are
literal event lists. Use AST analysis to retain atom literals, but reject an
entire event list when it contains runtime values such as map_id instead of
filtering individual tokens. Preserve the existing wanderer_app filtering and
MapSet collection behavior.
…tale
Two defects in the sidecar key added earlier on this branch, both of the
same family this branch is about.
No TTL. `WandererApp.Cache` is Nebulex with no default expiry, so a key
that never reaches the drain lives until the node restarts. Now written
with an explicit 30-minute ttl — well past the 5-minute drain interval,
short enough that an orphan disappears on its own.
Not cleared on re-track. The track branch calls
`remove_from_untrack_queue/2`, cancelling the queue entry, but left the
reason behind. A character untracked and re-tracked inside the drain
window therefore leaked the key *and* left a stale cause that could
label an unrelated untrack later. Deleted alongside the queue entry.
Also documents the dedupe race at the drain site: the queue dedupes on
{map_id, character_id} while the sidecar is last-writer-wins, so two
untracks with different causes in one window yield one untrack labelled
with the later cause. Correct and bounded, but worth not rediscovering
from scratch.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/wanderer_app/character/tracker_manager_impl.ex (1)
400-411: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRetain failed untracks for retry.
The queue entry and reason are removed before
Tracker.update_settings/2,MapCharacterSettingsRepo.update/3, and the remaining updates.Tracker.update_settings/2has reachable error results elsewhere in this module, but this task pattern-matches only{:ok, ...}. The task exit is logged at Lines 449-452, and the pair is not requeued.Keep the tuple and sidecar until all required updates succeed. Delete the sidecar only after successful processing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/wanderer_app/character/tracker_manager_impl.ex` around lines 400 - 411, Update the untrack processing flow around Tracker.update_settings/2 to retain the untrack queue entry and reason sidecar until Tracker.update_settings/2, MapCharacterSettingsRepo.update/3, and all remaining updates succeed. Handle failed update results without matching only {:ok, ...}, and ensure failures leave the tuple and reason available for retry; delete the sidecar only after successful processing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/wanderer_app/character/tracker_manager_impl.ex`:
- Around line 205-209: Restrict add_to_untrack_queue/3 to the supported reason
atoms :presence_driven, :acl_revoked, and :manual_untrack at its public
boundary. Either make the helper private if external callers are not required,
or validate the reason before calling WandererApp.Cache.insert, preserving the
existing nil behavior and rejecting all other terms.
---
Outside diff comments:
In `@lib/wanderer_app/character/tracker_manager_impl.ex`:
- Around line 400-411: Update the untrack processing flow around
Tracker.update_settings/2 to retain the untrack queue entry and reason sidecar
until Tracker.update_settings/2, MapCharacterSettingsRepo.update/3, and all
remaining updates succeed. Handle failed update results without matching only
{:ok, ...}, and ensure failures leave the tuple and reason available for retry;
delete the sidecar only after successful processing.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 04c34bf3-f121-498c-802e-2fed3ac1f564
📒 Files selected for processing (1)
lib/wanderer_app/character/tracker_manager_impl.ex
Review follow-up on #148. `untrack_reason/1` accepted any atom and handed it straight to `location_flag_cleared` as a Prometheus label and to `last_cleared_reason` as tracker state, while prom_ex_plugin.ex declares exactly four. Today every reason that reaches it comes through `CharactersImpl.untrack_characters/3`, whose guard already bounds the set — but that guard is one caller away from being bypassed, and the label is produced here, not there. Bounded at the point of production, so it covers every caller of `update_settings/2`. An unrecognised atom degrades to `:unknown` instead of raising: the outer boundary still raises, and crashing a character's tracker over a metric label would turn a cardinality slip into the exact freeze this instrumentation exists to detect. `add_to_untrack_queue/3` is now private. It had one caller, internal; a public arity-3 entry point was a second, unguarded way to set the label. The arity-2 form stays public — it takes no reason, and an integration test calls it directly.
guarzo
changed the title
guarzo
marked this pull request as draft
August 11, 2026 20:05
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to #146 and #147. Those shipped the fix and turned the meters on;
this makes the meters say something true, and fixes two defects found while
reading them.
The untrack reason was a constant wearing a variable's name
untrack_characters/2hardcodedreason: :presence_expiredon the:stoppedtelemetry event. Three separate branch points reach it — presence departure,
ACL revocation, and the admin Untrack button — and all three reported as
presence expiry. The label existed, so the metric looked dimensioned; every
query grouped by it got one answer.
The reason is now an argument, bounded to
:presence_driven | :acl_revoked | :manual_untrackby a guard, so a typoraises rather than quietly minting a new Prometheus series.
One step was non-obvious. The presence path does not call the tracker directly
— it queues
{map_id, character_id}for a drain five minutes later, and thattuple is the queue's uniqueness key under
Enum.uniq_by. Widening it wouldhave broken every reader, so the reason rides in a sidecar cache key the drain
reads and deletes.
The tracker keeps it as
last_cleared_reasonand reports it onlocation_flag_repaired, so the repair counter says what it repaired.How to read the metrics after this
repaired{reason=presence_driven}is the fix saving a character who would havefrozen on the map.
repaired{reason=manual_untrack}is the fix reverting anoperator who deliberately pressed Untrack. Only the first belongs in the
headline number.
manual_untrackwill not go to zero after this branch, and shouldn't:track_character/2deliberately re-tracks a character whose settings saytracked: falseon their next map entry with valid tokens and permissions. Adrop to zero would mean auto-restore had broken.
stoppedis now registered — emitted on every untrack since long before theplugin existed, collected nowhere. It's the denominator:
cleared / stoppedisthe share of untracks that caught a character still online in EVE.
Use
sum(increase(...[7d])) by (reason)— single Fly machine, so a deployresets the raw counters.
The admin Untrack button did half its job
The handler called
Map.Server.untrack_characters/2and nothing else. Thetracker stopped, but the
map_character_settingsrow still saidtracked: true, so every reader that consults settings — including this page'sown button, which stayed lit — reported the character as tracked. Success
flash, no visible change anywhere.
The row is now written, and the rendered
trackedderives from those rowsrather than from the presence cache. The second half isn't cosmetic: this page
untracks other users' characters and cannot write another user's presence
entry, so reading presence here could never reflect the action it had just
taken.
Both files are byte-identical to
upstream/main— inherited defect, not forkdrift, and a candidate for a separate upstream PR.
Two more instances of the same shape, plus tests that catch it
The recurring pattern in this investigation: source states an intent, the
runtime discards it, nothing disagrees out loud.
[:wanderer_app, :esi, :error]had a counter declared and no emitteranywhere in
lib/. The series never materializes, so its panel reads "nodata" — visually identical to "ESI is healthy", and an alert on it can never
fire. Now emitted from the six failure branches, with numeric ids stripped
from the endpoint label so the path doesn't open a series per character.
Every cache in
application.exwas declared withdefault_ttl:, a Cachex2.x option. This project runs 3.6, which ignores unrecognised options.
Fifteen caches claimed an expiry policy and applied none; two comments
elsewhere had already been written reasoning from those TTLs as real (both
corrected here).
test/unit/telemetry_registration_test.exschecks the telemetry contract inboth directions. It found 87 gaps, not one — 85 events emitted and collected
nowhere are recorded as a baseline the test holds the line against, with a
third test that fails if an entry goes stale.
test/unit/cache_options_test.exsfails if a dead Cachex 2.x option reappears.Both read source rather than starting the app: a dead option is dead whether or
not the cache starts, and an event reachable only under rare runtime conditions
still has to be declared in both places.
Decision left open, deliberately
The
default_ttl:options are removed, not translated. Turning realexpiration on across fifteen caches at once is a production behaviour change
that deserves a per-cache decision —
:character_state_cacheat 1h could evictan idle character's state and produce a freeze indistinguishable from the bug
this branch is chasing. The intended TTLs are recorded in a comment above the
list so restoring any of them is a decision, not an archaeology exercise.
Verification
mix test— 1778 tests, 0 failures.mix format --check-formattedandmix compile --warnings-as-errors --forceboth clean.Where to look
The sidecar-key trick in
tracker_manager_impl.exis the least obvious part ofthe diff. The
application.exdiff is large but almost entirely the commentblock plus reflowing after the option removal.
Summary by CodeRabbit
Bug Fixes
Monitoring