updating crawler watchdog branch to test - #18
Merged
Conversation
allowing more active sessions to show on active-sessions page
Lockfile churn from a prior `npm install` that resolved newer patch versions of existing dev dependencies (eslint internals and friends). Isolated in its own commit so it stays separable from the Vitest install that follows.
Captures the recon findings that contradicted the original brief (the ended_at cutover already shipped; isFullClear is dead code; the leaderboard SQL lives in leaderboard-cache.ts; request() has no retry logic) along with the ten decisions taken in response. Written down because the commits that follow will not match the brief they came from, and the reason should not live only in a chat log.
Vitest over Jest: it runs TypeScript and ESM natively with no babel or ts-jest layer, reads the existing tsconfig's path aliases, and does not contend with Next.js 16's bundler. - vitest.config.ts: node environment, colocated src/**/*.test.ts plus tests/**/*.test.ts, explicit @/ -> src/ alias mirroring tsconfig, v8 coverage reporter with no thresholds. - npm test / test:watch / test:coverage. - Renamed test-maintenance-cycle -> e2e:maintenance (same command) so `npm test` means fast, hermetic, no network and nothing else. The only other references to the old name are in untracked .codex/ transcripts. - Global network guard fails any test reaching the real internet without stubbing fetch, with its own tests covering the re-arm-after-stub case. Gate verified: a passing run, then a flipped assertion failing with a readable diff, then the throwaway deleted. eslint and tsc --noEmit both clean on the new files; no flat-config override was needed.
Fixtures are captured from the live API rather than hand-authored so they encode Bungie's real quirks — absent startingPhaseIndex, entry counts above six, activity durations that disagree with per-player time. A synthetic PGCR would only encode our beliefs about the API, which is what the fixtures exist to check. Every instance ID was selected by querying the local database for runs that actually exhibit the target property, so each case is a real observed run. The non-raid fixture probes forward from a known raid id until isRaidActivityHash rejects one. The script prints the salient fields per capture (entry count, completion count, fromBeginning, duration vs max time played, missing names) so a capture that fails to exhibit its stated property is visible rather than assumed. Run by the maintainer via `npm run capture-fixtures`; the API key is read from .env by the script and never handled directly.
Test databases are real SQLite files in a per-file temp directory, not `:memory:`. SQLite silently downgrades journal_mode=WAL to 'memory' for in-memory databases, so `:memory:` would exercise different journal semantics than production and undercut the reason for using a real database at all. Verified, not assumed. See ADR 0003. The path is set in a setupFile because DB_PATH is a module-level const resolved at import time; setting it there rather than inside a helper means test files can use ordinary static imports. It also relocates DATA_DIR, isolating maintenance-state.json — getDb() reads that file on every single call and would throw DatabaseMaintenanceError across the whole suite if the real database were mid-vacuum. Seeding goes through insertFullPGCR rather than raw INSERTs, because that is where ended_at is derived and last_seen_at advanced. Raw inserts would produce rows production could never create. Fixtures for realism, builders for permutation.
Against a real database, not a mock — better-sqlite3 opens a fresh one in about a millisecond, and mocking would validate the scaffolding instead of the SQL. Covers membership (checkpoint runs, zero-completion runs, non-finishers, and runs with no derivable end time are all excluded), the cutoff boundary judged on end time rather than start, raid filtering, the three-key tie-break, competition-style rank assignment, and the display name path. Two notes on the brief this came from: fullClearsOnly has no false branch to test — it is forced true on every path — and the SQL lives in lib/cache/leaderboard-cache.ts, not lib/db/queries.ts. One test pins a defect rather than asserting correctness: formatDisplayName drops the #Code when the code is 0, because it guards with a truthiness check. Reported, not fixed.
The brief asked for parity tests guarding an in-flight cutover from the run_durations CTE to pgcrs.ended_at. That cutover already shipped in 610408e and no run_durations SQL remains in src/, so there is nothing left to guard. Retargeted to the code that replaced it: the three-tier duration fallback in computeActivityDurationSeconds and the future-end-time corruption guard, asserted both directly and through insertFullPGCR. Covers Tier 1 winning over per-player time, a zero duration being treated as unusable, late-joiner offsets, non-completers counting toward duration (the CTE excluded them), the Tier 2 collapse when start offsets are absent, Tier 3 yielding NULL, the future-end-time discard with its clock-skew headroom, and last_seen_at advancing monotonically.
Mocked at the fetch boundary only. Stubbing our own modules would test the stubs; stubbing fetch tests the response handling that decides whether a raid gets ingested or dropped. Correction to the brief: request() has no retry logic. It classifies a failure and, when Bungie signals throttling, pauses the shared per-key rate limiter — the throttle applies to the key, not to the one request that saw it. The tests follow that actual behaviour. - maintenance: the SystemDisabled predicate, with its negative cases (privacy restriction, plain 5xx, timeout, error-shaped impostor) carrying as much weight as the positive one. A false positive pauses the whole crawler for a blip; a false negative burns thousands of doomed requests. - client: payload handling, the typed-vs-untyped error distinction that the maintenance predicate depends on, non-JSON Cloudflare bodies, and each of the four pause paths (ThrottleSeconds, 429 with and without Retry-After, and the 1672 game-server backoff that reports no duration). - rate-limiter: FIFO serialization, and pauses that catch queued waiters, extend a sleep already in progress, and never shorten an existing pause. Timing is asserted with fake timers, so the suite stays fast.
First workflow in the repo. Node 22, matching local and production. Adds `tsc --noEmit` beyond the brief: nothing else typechecks the repo, since vitest transpiles without checking and a full `next build` is too slow for a per-push gate. The tsconfig includes **/*.ts, so test files are checked too. The e2e maintenance harness is excluded — it spawns long-running processes against a mock Bungie server. Documented in the workflow: CI is x86_64 while production is ARM64 (Oracle A1 Flex), so native-module differences in better-sqlite3 will not be caught here.
The brief made ProcessedPGCR.isFullClear the centrepiece here, calling its three-way || the riskiest logic in the codebase. It is dead code: computed, returned, and never read. fetchAndStorePGCR persists Bungie's raw activityWasStartedFromBeginning instead, and every leaderboard filters on that column. It is also wrong. Bungie no longer sends startingPhaseIndex (absent on all 827k stored rows), so the `=== undefined` branch fires unconditionally and reports every run as a full clear — including the 568k checkpoint runs. Harmless only because nothing consumes it; wiring it up would inflate every leaderboard by roughly 2.2x. So Phase 3 tests the signal that actually ships: - pgcr.test.ts: the per-entry .some() completion check, director hash vs referenceId fallback, raid key resolution, ISO-to-unix conversion (including a DST boundary, to pin that no local offset leaks in), and player extraction tolerating a withheld global display name. - full-clear-flag.test.ts: that checkpoint runs persist as 0, that starting_phase_index is inert, and that the full-clear flag stays independent of whether anyone finished. isFullClear is left untested on purpose, with the reasoning recorded in the test file — pinning dead behaviour would only make its removal harder. Flagged for deletion in a future change, not deleted here.
ADR 0003 — test databases are real SQLite files, not `:memory:`. SQLite silently downgrades journal_mode=WAL to 'memory' for in-memory databases, so `:memory:` would exercise different journal semantics than production and give up most of the reason for using a real database. ADR 0004 — mock only at the network boundary. No vi.mock of our own modules, real database, seeding through insertFullPGCR. Written down because mocking one's own modules is the default habit elsewhere, so the absence needs explaining before someone adds it back. CONTEXT.md — Full Clear sharpened to name the only authoritative signal, since the codebase held two competing implementations of the term and the glossary did not say which was canonical. Added Checkpoint Run (the majority of observed raids, previously unnamed) and Completion (the unit leaderboards actually rank by, and narrower than "finished a raid"). decisions.md — the recon findings that reshaped the work, plus four defects reported rather than fixed: dead-and-wrong isFullClear, formatDisplayName dropping #Code on a zero code, getDb() reading the maintenance state file on every call, and CLAUDE.md describing a runtime manifest dependency that does not exist. Also brings ADRs 0001 and 0002 into git; they were written earlier but never committed.
Eight real Bungie responses, captured verbatim. Capturing them corrected three claims that had been inferred from the database rather than observed at the source. The conclusions held; the mechanisms did not. - startingPhaseIndex is SENT, always 0 — not absent. Present on every fixture including three confirmed checkpoint runs. The database showed 0 everywhere because the writer coerces with `|| 0`, which had flattened the evidence. So isFullClear's `=== 0` branch fires, not its `=== undefined` branch. Still true for 100% of runs; the defect is unchanged. Corrected in the docs and test comments. - A six-entry raid report can belong to two players with three characters each. pgcr_players is keyed (instance_id, membership_id) with INSERT OR IGNORE, so only each player's first entry survives — 981s stored where their longest character played 1494s. Those columns are read nowhere in src/, so it is latent. Duration derivation is unaffected: it runs before the dedupe. - Bungie can withhold identity entirely: nineteen entries, all isPublic false with membershipType 0 and no name field at all. Not a missing global name with a platform fallback — no fallback exists. Ingestion stores all nineteen with NULL names, which is correct. Two fixtures renamed to what they actually are: the "duration divergence" case is really the multi-character case (the gap is 8%), and the "no duration" case is really an absurd 27384s duration on an 18-minute run. There is no true Tier 3 fixture; builders cover it.
Test framework
…ock-aware The two counts now navigate to /leaderboard and /active-sessions. They carry no styling of their own — colour, weight and underline all inherit, so the strip is pixel-identical for mouse and touch users; a focus-visible ring is the only addition, and it fires for keyboard navigation only. Deletes FooterStatus, whose "Updated Xs ago" the StatsBar ticker supersedes. These were never duplicates, though: the footer reported the *crawler heartbeat* (how stale the data is) while the StatsBar ticker reports *this tab's last fetch* (how stale the page is). They diverge exactly when it matters — a dead crawler left the bar reading "Updates paused - Updated 8s ago". So the freshness slot now picks its clock: page age while live, data age once the heartbeat lapses, and nothing at all when the crawler is down and its age is unknown (the "Maintenance" label already says everything true there). That three-way choice is extracted to selectFreshness() so it can be tested under vitest's node environment, which has no DOM by design. Docs: CONTEXT.md gains Heartbeat / Data Freshness / Page Freshness — the missing vocabulary that let one phrase mean two things in the first place. ADR 0002 notes that its count-vs-list rule now extends to navigation: the links deliberately don't override a reader's saved time range or raid filters.
feat: link StatsBar counts to their pages; make the freshness slot cl…
Resolves two doc conflicts: - tests/README.md: both sides documented the DB_PATH guard at the same anchor. Kept main's fuller explanation of why setupFiles order is load-bearing, followed by the branch's recovery note for the error. - docs/decisions.md: the branch's note cited a code-layout line and a 'Raid Detection' section that main's CLAUDE.md trim has since removed. Reworded to point at the surviving conventions bullet.
Test guard db
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.
No description provided.