fix(app): let an explicit refresh bypass an in-flight api() request (#243) by dborup-agent · Pull Request #263 · dborup/CoreScope

dborup-agent · 2026-10-05T14:32:44Z

Relates to #243

Problem

api(path, { bust: true }) skipped only the TTL cache, not the _inflight map. invalidateApiCache('/channels') does not clear _inflight either.

When a channel is approved (admin decision, or auto-approve since #238), onApproved refreshes the list. If a /channels request is already in flight at that moment, the refresh joins it. That can happen after a region change, the encrypted toggle, or a second approval. The older request's answer can predate the approval, so the approved channel stays missing until the next refresh.

Fix

public/app.js api()

bust: true never joins an in-flight request. It always fetches, and it takes the in-flight slot for its key, so later callers without bust join the newer request.
A request that a bust has superseded is left in a passive state:
- it does not write the TTL cache. Otherwise its late answer would overwrite the newer data for the rest of the TTL;
- its .finally() removes the _inflight entry only if that entry is still its own.
Calls without bust are unchanged. For them the slot is always their own promise, so both checks are always true.

public/channels.js (kept minimal; #257 also edits this file)

loadChannels(silent, opts) passes opts.bust on to api().
onApproved calls loadChannels(true, { bust: true }). invalidateApiCache('/channels') stays, so the cached lists for other region and includeEncrypted variants are still cleared.
The channelsRequestId token from fix(channels): drop stale channel-list responses and show the unread badge on mobile (#154, #155) #225 is unchanged, so an older response is still dropped without a render or a reconcile.

Other `api()` callers

No other caller of the /channels list endpoint in channels.js passes bust: region change, the encrypted toggle and init() are unchanged.
channels.js already has a bust: !!opts.forceNoCache passthrough, but it is on a different endpoint and pre-dates this PR: refreshMessages()'s fetch of /channels/{hash}/messages (region-switch handler, from test(server): data race between background index builds and the hash-migrate log capture (#301) #307). That is the messages endpoint for the open conversation, not the /channels list this PR's race is about, and it is unaffected by this change.
The only other bust caller of a list-shaped endpoint is the manual refresh in observers.js. It now gets fresh data instead of joining an in-flight auto-refresh. This only happens when the two overlap, and it is what its fix(observers): show "Last updated" timestamp on aggregate header (closes #1562) Kpa-clawbot/CoreScope#1563 request-id guard already expects. The observers unit tests and E2Es are green.

Normal case

With nothing in flight, an approval still makes exactly one /channels request. A test pins this.

Overlap with #257

#257 moves the onApproved body into refreshChannelList(). Whichever PR merges second needs a one-line resolution: loadChannels(true, { bust: true }) goes into refreshChannelList(). The new tests sit away from #257's test hunks.

Update (round 2): #257 merged first. master was merged into this branch and the conflict was resolved exactly as predicted above — refreshChannelList() now does invalidateApiCache('/channels'); loadChannels(true, { bust: true });, and both onApproved and onRevoked wire to it. See the PR comment for the merge-resolution writeup.

Tests

The tests were committed first (899e9232) and are red on master. The fix is in 601bd000.

Test	Checks	Red on master
`test-app-api-bust-inflight-243.js` (new, 7)	Uses the real `app.js` with parked fetches. A bust during an in-flight request fetches again and gets the newer data. A later plain call joins the newer request. The older request settling first does not remove the newer entry. An older answer landing last does not overwrite the cache. A bust with nothing in flight makes one fetch. Guards: plain dedup and TTL, a failed bust leaves no entry	4 of 7 (the 3 guards pass)
`test-channels-client-state-152.js` (+3)	Uses the real `channels.js` with the real `app.js` `api()`. An approval while a region load is in flight lists and renders the approved channel, in both answer orders, with one request for each load. The next refresh within the TTL still has the channel, with no request. Nothing in flight → exactly one request	2 of 3 (the normal-case guard passes)
`test-channel-proposals.js` (+1)	`ChannelProposals.unmount()` cancels the suggest poller: no poll stays scheduled, and there is no status request afterwards	green: pins existing behaviour (the test gap from the #238 review)

Mutants

Each mutant was applied to the fixed code and run against the three files.

Mutant	Killed by
`api()` ignores `bust` for `_inflight`	api 4 ✗, channels 2 ✗
`onApproved` without `bust`	channels 2 ✗
`loadChannelsFor` does not pass `bust` on	channels 2 ✗
cache-write ownership check removed	api 1 ✗, channels 1 ✗
`.finally()` ownership check removed	api 1 ✗
#225 stale-response check removed	channels 4 ✗ (3 existing #154 tests and 1 new)
`state.suggestPoller.cancel()` removed	proposals 1 ✗

Local runs (head `601bd000`)

sh test-all.sh: 220 / 220 files.
node test-frontend-helpers.js: 707 / 0.
node test-channel-proposals.js: 37 / 0.
node test-channels-client-state-152.js: 67 / 0.
Against a local Go server on e2e-fixture.db, prepared as in CI (freshen, the Packets page collapse button in the left column of the table opens the dialog. Kpa-clawbot/CoreScope#1486 seed, migrate, the [feat] node detail, separate flood-adv from zero-hop adv Kpa-clawbot/CoreScope#2073 and Active observers vanish as nodes after retention.nodeDays without an advert; observer link dead-ends on 'Node not found' #199 seeds):
- 23 channel and observer E2Es;
- test-e2e-playwright.js: 132 / 135, 3 skipped (built-in skips).
With a self-started server and ingestor: test-channel-proposals-e2e.js (24 / 0) and test-channels-client-state-152-decrypt-e2e.js (18 / 0).
scripts/check-xss-sinks.sh --diff origin/master: clean.
No CSS or colour changes, and no workflow changes. The fork guards are unchanged: 9 in deploy.yml and 1 in release-fast-path.yml.

Perf

Not a hot path. api() adds one boolean test and, per response, one Map.get. An extra request only happens when an explicit refresh overlaps an in-flight request for the same path. That is the request the refresh asked for.

Not verified

The race itself was not reproduced on a live instance. It was reproduced with controlled fetch timing in the unit tests.

🤖 Generated with Claude Code

…pi() request (#243) - test-app-api-bust-inflight-243.js: api(path, { bust: true }) during an in-flight request must fetch again, take the in-flight slot, and keep its newer data in the TTL cache when the older answer lands last. - test-channels-client-state-152.js: the real channels.js with the real app.js api(): an approval while a region load is in flight must list the approved channel, in both answer orders; nothing in flight makes exactly one request. - test-channel-proposals.js: ChannelProposals.unmount() cancels the suggest poller (no scheduled poll, no status request afterwards). Red on master: 4 api() tests and the 2 in-flight channel tests. The unmount test is green on master and red without suggestPoller.cancel(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…243) api(path, { bust: true }) skipped only the TTL cache. A refresh asked for while the same request was in flight joined that older request, whose answer may predate the change (an approval), so the approved channel was missing until the next refresh. - api(): a bust never joins _inflight; it fetches and takes the in-flight slot, so later plain callers join the newer request. A superseded request neither writes the TTL cache nor removes the newer slot entry. Calls without bust are unchanged. - channels.js: onApproved refreshes with loadChannels(true, { bust: true }). The #225 request token still drops the older response. The only other bust caller, the observers manual refresh, now gets fresh data instead of joining an in-flight auto-refresh, as its Kpa-clawbot#1563 request guard already expects. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

dborup-agent · 2026-10-05T15:23:04Z

Rapport — CS-pve-agent2 PR#263 #243 — head `601bd00`

Status: fix and tests done; local suites and CI green (no re-runs).

Evidence tags: [T] = test or command run in this session, [A] = analysis/reading of code, [K] = existing knowledge, not re-checked here.

Requirements

#	Requirement	Test	Mutant	Result
1	A refresh during an in-flight `/channels` request lists the approved channel; no extra request in the normal case	`test-channels-client-state-152.js`: `#243: approval while a /channels request is in flight…` (both answer orders), `#243: approval with nothing in flight makes exactly one /channels request`	`api()` ignores `bust` → 2 ✗; `onApproved` without `bust` → 2 ✗; `loadChannelsFor` drops `bust` → 2 ✗	pass [T]
2	The #225 `channelsRequestId` token is kept	the same test (older answer lands last) plus the existing #154 tests	stale check removed → 4 ✗	pass [T]
3	Unit test with controlled `api()` timing	the channel tests above use the real `app.js` `api()` with parked fetches; `test-app-api-bust-inflight-243.js` (7 tests)	`bust` ignored → 4 ✗; cache-write ownership check removed → api 1 ✗ and channels 1 ✗; `.finally()` ownership check removed → api 1 ✗	pass [T]
4	`unmount()` cancels the suggest poller	`test-channel-proposals.js`: `unmount() cancels the suggest poller…`	`state.suggestPoller.cancel()` removed → 1 ✗	pass [T]
5	Red before the fix	at `899e9232` (tests only): 4/7 api tests and 2/3 channel tests are red [T]. The unmount test is green on master, because it pins existing behaviour, and red only against the mutant [T]	–	as stated
6	Other `api()` callers are unchanged	Guard tests cover plain-call dedup and TTL. `sh test-all.sh` 220/220, `test-app-api-inflight-cleanup-rejection.js` PASS, observers E2Es green [T]. The one intended change: the manual refresh in `observers.js` (the only other `bust` caller) no longer joins an in-flight auto-refresh [A]	–	pass
7	No hardcoded colours or per-item calls; XSS gate; fork guards	`bash scripts/check-xss-sinks.sh --diff origin/master` exits 0 with no findings [T]. No CSS changes [T]. Fork guards: 9 in `deploy.yml`, 1 in `release-fast-path.yml`, no workflow diff [T]	–	pass

Local runs [T]

sh test-all.sh: 220/220 files.
node test-frontend-helpers.js: 707/0.
node test-channel-proposals.js: 37/0.
node test-channels-client-state-152.js: 67/0.
Against a local Go server on e2e-fixture.db, prepared as in CI: 23 channel and observer E2Es all pass. test-e2e-playwright.js: 132/135, 3 built-in skips.
Self-started: test-channel-proposals-e2e.js 24/0, test-channels-client-state-152-decrypt-e2e.js 18/0.
The server was stopped by port.

CI (run 37325629702)

Job	Result
Go Build & Test	success [T]
Playwright E2E Tests	success, first attempt; the #256 flake did not occur [T]
Build & Publish Docker Image	success [T]
Release Artifacts / Deploy Staging / Publish Badges & Summary	skipped (PR event) [T]

Remaining

Overlap with fix(channels): hide revoked shared channels from the channel list (#251) #257: whichever PR merges second moves loadChannels(true, { bust: true }) into refreshChannelList() (a one-line resolution) [A].
The race was not reproduced on a live instance, only with controlled timing in unit tests.

dborup-agent · 2026-10-05T15:24:25Z

CI update — CS-pve-agent2 PR#263 #243 — head `601bd00`

Status: CI green, no re-runs needed.

Run 37325629702 [T]:

Job	Result
Go Build & Test	success
Playwright E2E Tests	success (first attempt; the #256 flake did not occur)
Build & Publish Docker Image	success
Release Artifacts	skipped (PR event)
Deploy Staging	skipped (PR event)
Publish Badges & Summary	skipped (PR event)

What remains is the one-line overlap with #257 (see the report above).

dborup · 2026-10-05T18:36:54Z

Review — CS-MacBook PR#263 — head `601bd00`

Dom: REQUEST CHANGES

Evidence tags: [T] = test/command run in this session, [A] = analysis/reading of code, [K] = existing knowledge, not re-checked here.

Finding table

#	Finding	Severity	Evidence
1	PR no longer merges cleanly: #257 merged into `master` today (merge commit `c6b356de`, now `origin/master` tip) and `git merge-tree --write-tree origin/master 601bd000` produces a real `CONFLICT (content): Merge conflict in public/channels.js` (two hunks: the `onApproved`/`onRevoked` wiring block, and the `loadChannelsFor` signature / `refreshChannelList` block). The PR description anticipated this as a "one-line resolution" against an unmerged #257; it is now a real, current conflict against `master`, not a hypothetical.	Blocking	[T]
2	PR description says "No other caller in `channels.js` passes `bust`" / "the only other `bust` caller is the manual refresh in `observers.js`" — inaccurate: `channels.js` already has a pre-existing `bust: !!opts.forceNoCache` passthrough for `/channels/{hash}/messages` (unaffected functionally, since the fix is a global, non-breaking improvement for every `bust` caller, but the claim itself is wrong).	Nit	[T]

No correctness defects found in the api()/channels.js fix itself — see below.

Point-by-point

1. api() bypass. Implemented as bust: true [A] (public/app.js): it skips the _inflight.has() join check, takes the in-flight slot via the unconditional _inflight.set(inflightKey, promise), and two new ownership guards (_inflight.get(inflightKey) === promise) stop a request a bust has superseded from (a) writing the TTL cache and (b) deleting a newer entry on .finally(). Traced both answer orders by hand against the implementation; matches the new unit tests exactly [A]. Other api() callers: the !bust path is unchanged — same TTL-cache short-circuit, same _inflight.has() join, same delete-on-finally, modulo the ownership check, which can only diverge from "always true" when a concurrent bust took the slot (a tightening, not a behavior change for the common case) [A].

2. #225 token (channelsRequestId). Unchanged line: if (requestId !== channelsRequestId) return latestChannelsLoad; in loadChannelsFor [A]. Independently removed it as a mutant on the merged tree (see Mutants) — kills 3 pre-existing #154 tests plus one new #243 test (older-answer-lands-last), confirming the token still backstops the api()-level fix rather than being redundant with it [T].

3. Tests.

test-app-api-bust-inflight-243.js: checked out the tests-only commit 899e9232 — 4/7 red (3 guard tests green, as the PR claims) [T]. At 601bd000: 7/7 green [T].
test-channels-client-state-152.js #243 block: at 899e9232 — 2/3 red (the "nothing in flight" guard passes, as claimed) [T]. At 601bd000: 3/3 green [T].
unmount() cancels the suggest poller: green already at 899e9232 (pins pre-existing, untouched channel-proposals.js behavior, consistent with the PR's own description) [T]. Independently mutated state.suggestPoller.cancel(); → removed it → the new test fails (1/40), the other 39 stay green [T].

4. Conflicts (#257). #257 ("hide revoked shared channels," codex/issue-251-hide-revoked-channels) is merged — gh pr view 257 shows state: MERGED, merge commit c6b356de, which is origin/master's current tip [T]. This supersedes the task brief's premise that #257 was "approved in round 2, not merged." Real conflict confirmed via git merge-tree --write-tree origin/master 601bd000a5aa382abf48489f72a8705f271bf86f (exit 1, CONFLICT (content): Merge conflict in public/channels.js) [T]. To verify the underlying fix is still sound post-#257, built a worktree off origin/master, merged in 601bd000, and resolved the conflict exactly as the PR description's own "one-line resolution" predicts: kept onApproved: refreshChannelList / onRevoked: refreshChannelList from master, and folded bust: true into refreshChannelList()'s loadChannels(true, { bust: true }) call, keeping the loadChannelsFor(requestId, silent, bust) signature from #263 [A]. On that resolved tree: all public/*.js unit suites, sh test-all.sh (220/220), and the full channel E2E battery (below) pass [T]. So the fix composes correctly with #257 once rebased — but #263 as currently pushed cannot be merged as-is.

5. Channel E2Es + test-channel-proposals-e2e.js on the merged tree. All green against a local Go server (cmd/server, cmd/migrate, cmd/ingestor built from the resolved merge tree) on port 13901, fixture freshened + migrated + seeded (2073, 199) per the CI steps [T]:

test-channels-client-state-152-e2e.js 6/6, test-channels-154-155-e2e.js 8/8, test-channel-issue-1087-e2e.js 3/3, test-channel-issue-1111-e2e.js 2/2, test-channel-fluid-e2e.js 15/15, test-issue-1224-channels-mobile-ux-e2e.js 16/16, test-issue-1367-channels-chat-app-e2e.js 9/9, test-issue-1657-analytics-channels-group-sprites-e2e.js 5/5.
test-channel-proposals-e2e.js (self-starting ingestor+server): 26/26, including the exact approve-while-busy → shared-channel-visible path, revoke/restart, and auto-approve scenarios.
test-channels-client-state-152-decrypt-e2e.js (self-starting): 18/18.
A broader test-e2e-playwright.js run stopped early on an unrelated pre-existing fixture gap (#1791 Group Data filter test needs a seed row the task's prep steps — freshen + seed-2073 + seed-199 — don't include); not a PR regression, and out of the channel-E2E scope asked for [A]/[T].

Tests and mutants run this session

cmd/server: go test ./... — pass (40.9s) [T].
cmd/ingestor: go test ./... — pass (107s) [T].
sh test-all.sh on the resolved merge tree — 220/220 [T].
node test-frontend-helpers.js — 707/0 [T].
node test-app-api-bust-inflight-243.js, test-channels-client-state-152.js, test-channel-proposals.js — all green on both the raw head and the resolved merge tree [T].
bash scripts/check-xss-sinks.sh --diff origin/master on the PR head — exit 0, no findings [T].
Fork guards: 9 in deploy.yml, 1 in release-fast-path.yml, git diff origin/master...601bd000 -- .github/workflows/ empty [T]. No closing keywords in the commit message or PR body [T]. Commit author: dborup <kontakt@meshview.dk> [T]. cmd/server untouched by the diff; no new map[string]interface{} outside tests (diff stat touches only public/app.js, public/channels.js, test-all.sh, three test-*.js files) [T]. No CSS/color changes [T].
CI (run 37325629702): Go Build & Test, Playwright E2E, Build & Publish Docker Image all pass [T] — reconfirmed via gh pr checks 263.
Mutants (own, independent of the author's list):
1. app.js: reverted the !bust && guard on the _inflight.has() join check → 4/7 api-bust tests fail, 2/3 #243 channel tests fail [T].
2. On the resolved merge tree: dropped bust: true from refreshChannelList()'s loadChannels() call → both #243 in-flight channel tests fail [T] (confirms the predicted one-line rebase fix is in fact necessary, not cosmetic).
3. channels.js: removed the #225 staleness check (if (requestId !== channelsRequestId) return latestChannelsLoad;) in the success path → 3 pre-existing #154 tests + 1 new #243 test fail [T].
4. channel-proposals.js: removed state.suggestPoller.cancel(); in unmount() → the new unmount test fails, nothing else does [T].

Not verified

The race was not reproduced against a live instance — only unit timing and the channel-proposals E2E's approve-path exercise it, consistent with the author's own report.
Did not run the full test-e2e-playwright.js battery to completion (stopped on an unrelated, pre-existing fixture-seed gap outside this PR's and this review's scope).
Did not attempt to resolve and push a rebase myself — read-only review, no push/merge per the task's instructions.

…-inflight-refresh # Conflicts: # public/channels.js

dborup · 2026-10-05T19:53:44Z

Rapport — CS-MacBook PR#263 runde 2 — head `59e398c`

Evidence tags: [T] = test/command run in this session, [A] = analysis/reading of code, [K] = existing knowledge, not re-checked here.

Review feedback addressed (commit `59e398cd`)

Merge conflict (finding 1, blocking). origin/master (c6b356de, tip at merge time, includes fix(channels): hide revoked shared channels from the channel list (#251) #257) merged into codex/issue-243-api-inflight-refresh with a real merge commit — no rebase, no force-push. The conflict in public/channels.js was resolved by hand, see hunk-by-hunk below. All other files auto-merged cleanly. [T]
PR description inaccuracy (finding 2, nit). Rewrote the "Other api() callers" section: it previously implied channels.js had no other bust caller besides observers.js. It actually already has a bust: !!opts.forceNoCache passthrough in refreshMessages() for the /channels/{hash}/messages endpoint (pre-existing since test(server): data race between background index builds and the hash-migrate log capture (#301) #307, region-switch handler) — a different endpoint than the /channels list race this PR fixes, and unaffected by this change. The description now says so explicitly, and a short "Update (round 2)" note was added to the "Overlap with fix(channels): hide revoked shared channels from the channel list (#251) #257" section recording that the predicted one-line resolution is what actually landed. [A]

Conflict resolution, hunk by hunk

Hunk 1 — ChannelProposals.mount() wiring (onApproved/onRevoked).
HEAD (#263) had onApproved: function () { invalidateApiCache('/channels'); loadChannels(true, { bust: true }); } and no onRevoked. origin/master (#257) had onApproved: refreshChannelList and added onRevoked: refreshChannelList. Resolved by taking master's wiring verbatim — both hooks point at refreshChannelList — and folding #263's bust behavior into refreshChannelList itself in hunk 2, so both approval and revocation refreshes bust the in-flight /channels request, matching the PR description's own predicted resolution.

Hunk 2 — refreshChannelList / loadChannelsFor signature.
HEAD had async function loadChannelsFor(requestId, silent, bust) with no refreshChannelList (its body lived inline in onApproved). Master had function refreshChannelList() { invalidateApiCache('/channels'); loadChannels(true); } plus async function loadChannelsFor(requestId, silent) (no bust param). Resolved to:

function refreshChannelList() {
  invalidateApiCache('/channels');
  loadChannels(true, { bust: true });
}

async function loadChannelsFor(requestId, silent, bust) {

keeping master's invalidateApiCache('/channels') call, #263's bust parameter and its passthrough to api('/channels' + qs, { ttl: CLIENT_TTL.channels, bust: bust }) further down (unchanged, outside the conflict region), and the untouched #225 staleness check if (requestId !== channelsRequestId) return latestChannelsLoad;. hiddenChannelNames assignment from data.hiddenChannels (#257) sits right after the api() call and was never part of the conflict — it merged automatically and is unchanged. [A]

Net effect: both an approval and a revocation now call refreshChannelList, which busts the in-flight /channels request exactly as #263 intended, while keeping #257's hidden-channels tracking and the #225 request-id token intact.

Tests [T]

All run against the merged tree (59e398cd), binaries built from it:

Test	Result
`node test-app-api-bust-inflight-243.js`	7/0
`node test-channels-client-state-152.js`	70/0 (3 more than the PR's original 67 — extra assertions from master's #257/other merges)
`node test-channel-proposals.js`	40/0
`sh test-all.sh`	220/220 files
`node test-frontend-helpers.js`	707/0
`cd cmd/server && go test ./...`	pass (40.5s)
`test-channel-proposals-e2e.js` (self-starting server+ingestor, auto-assigned port)	26/0
`test-channels-client-state-152-decrypt-e2e.js` (self-starting)	18/0

Channel E2Es against a shared local Go server (built corescope-server/corescope-migrate/corescope-ingestor from the merged tree, port 13581, fixture freshened + Kpa-clawbot#1486/Kpa-clawbot#1791 seeded + migrated + Kpa-clawbot#2073/#199/#245 seeded, exactly as deploy.yml's Playwright job does):

test-channel-issue-1087-e2e.js 3/0, test-channel-issue-1111-e2e.js 2/0, test-channels-client-state-152-e2e.js 6/0, test-channel-fluid-e2e.js 15/0, test-channel-decrypt-e2e.js 15/0, test-channel-qr-e2e.js 11/0, test-channel-color-picker-e2e.js 10/0, test-channel-modal-e2e.js 15/0, test-channels-154-155-e2e.js 8/0, test-issue-1657-analytics-channels-group-sprites-e2e.js 5/0, test-issue-1224-channels-mobile-ux-e2e.js 16/0, test-issue-1367-channels-chat-app-e2e.js 9/0, test-channels-list-render-e2e.js 8/0, test-channels-selection-flow-e2e.js 8/0, test-channels-add-modal-e2e.js 8/0, test-channels-share-color-e2e.js 6/0, test-channels-ws-batch-e2e.js 6/0, test-channels-ws-race-1498-e2e.js 5/0, test-channels-observed-path-hash-size-e2e.js 7/0. All pass, nothing skipped.

Fixture DB and build artifacts were reverted after the run (e2e-fixture.db.orig restored, binaries and the generated mbcap-snapshot snapshot removed) — git status on the merged tree is clean before the push.

Mutants [T]

Both applied to the resolved public/channels.js, run, then reverted (confirmed clean via git diff/git status afterward):

Mutant	Killed by
bust ignored: `api('/channels' + qs, { ttl: CLIENT_TTL.channels, bust: false })` in `loadChannelsFor`	`test-channels-client-state-152.js`: both `#243` in-flight-approval tests fail (68/2)
`hiddenChannels` guard removed: `} else if (isFirstObservation) {` (dropped `&& !hiddenChannelNames.has(channelKey)`) in `processWSBatch`	`test-channels-client-state-152.js`: "a live CHAN packet does not re-create the row of a hidden channel (#251)" fails (69/1)

Both mutants died as required.

CI (run 37360022795, head `59e398cd`)

Job	Result
Go Build & Test	pass (22m27s)
Playwright E2E Tests	pass (24m33s) — first attempt, #267/#271 did not flake
Build & Publish Docker Image	pass (45s)
Release Artifacts	skipping (PR event)
Deploy Staging	skipping (PR event)
Publish Badges & Summary	skipping (PR event)

Not done / out of scope

No merge, ready-for-review flip, approval, or revert — the PR remains draft, per the task's constraints.
No stg/prod access, no upstream writes, no other branches touched. All work happened in a scratch worktree (/Users/dborup/CoreScope-worktree-pr263), now safe to remove.
Did not re-run the broader test-e2e-playwright.js battery — out of scope for this round (only the channel E2Es and the specific test files listed in the task were requested).

adminopenclaw8-sketch · 2026-10-05T20:26:15Z

Review — CS-Macmini PR#263 — head `59e398c`

Dom: APPROVE med nits

Independent read-only re-review. Evidence tags: [T] = run in this session, [A] = code reading/analysis, [K] = taken from existing context, not re-checked.

Scope per the round-1 outcome: the round-1 fix was already found correct, and REQUEST CHANGES was raised only for the #257 merge conflict. This round therefore concentrates on the conflict resolution and on both PRs' behaviour surviving it.

Findings

#	Sev	Where	Finding
N1	nit	`public/channels.js:1226`	The revoke path's bust is only pinned transitively. A mutant that re-splits the wiring — `onRevoked` given its own non-busting `invalidateApiCache('/channels'); loadChannels(true)` — survives all three unit suites [T]. Dropping `bust` from the shared `refreshChannelList()` does die (M6), so the implementation itself is pinned; this is close to an equivalent mutant and only matters if a future change re-splits the two hooks.
N2	nit, pre-existing	`public/channels.js:2024`	Dropping `invalidateApiCache('/channels')` from `refreshChannelList()` survives all three suites [T]. I re-ran the equivalent mutant against `origin/master` (pre-`#263` `refreshChannelList`) and it survives there too [T] → a pre-existing #257 gap, not introduced here. Worth recording that after this PR the call's only remaining job is the other `region` / `includeEncrypted` cache variants: `bust: true` bypasses the exact-path entry by itself, and no test covers the cross-variant invalidation.
N3	nit, scope	`public/nodes.js:684`, `public/nodes.js:711`	The fix is applied at the call site (`bust`) rather than inside `invalidateApiCache()`, so the identical invalidate-then-plain-`loadNodes(true)` shape in the nodes advert WS handler still coalesces onto an in-flight `/nodes` request [A]. Correctly out of scope — the issue scopes itself to `/channels` — but the issue title names `invalidateApiCache` generally, so the residual is worth a follow-up rather than being silently closed with #243.
N4	nit, bookkeeping	issue #243 acceptance box 2	"Both new tests fail before the fix" does not literally hold: the `unmount()` suggest-poller test is green on master [T] because it fills the test gap the issue itself describes, with no production change behind it. Mutant kill is the right gate there, and the author's report says so plainly — noting it only so the checkbox is not read as a failed criterion.

No blocking findings. The conflict resolution matches what the PR description predicted and what the round-1 review asked for.

Answers to the review points

1. Conflict resolution in public/channels.js — all four behaviours preserved [A][T]

git merge-tree --write-tree origin/master 59e398cd produces tree 572fa5af with no conflict [T]. On that tree:

Must survive	Where	Status
#263 bust-refresh	`refreshChannelList()` → `loadChannels(true, { bust: true })` → `loadChannelsFor(…, bust)` → `api('/channels' + qs, { ttl: CLIENT_TTL.channels, bust: bust })`	present, unbroken chain (`channels.js:2024–2038`)
#257 `hiddenChannels` + `processWSBatch` guard	`hiddenChannelNames = new Set(…data.hiddenChannels…)` at `:2044`; `} else if (isFirstObservation && !hiddenChannelNames.has(channelKey)) {` at `:1789`	present and byte-identical to master — neither line appears in the master→merged diff
Revoke refresh with `invalidateApiCache('/channels')`	`onRevoked: refreshChannelList` at `:1226`, and `invalidateApiCache('/channels')` kept as the first statement of `refreshChannelList()` at `:2024`	present
#225 token	`channelsRequestId` bumped at `:2015`; `if (requestId !== channelsRequestId) return latestChannelsLoad;` on both the success path (`:2042`) and the catch path (`:2066`)	present, unchanged by the merge

Both hooks now point at one function, so approval and revocation bust — strictly a superset of each PR's own behaviour, and the resolution the PR description predicted.

2. Mutants — all three required ones die [T]. See the mutant table below (M1, M2, M3).

3. No changes beyond the conflict resolution [T]. git diff origin/master 572fa5af touches exactly the PR's six files, +322/−12: public/app.js, public/channels.js, test-all.sh, test-app-api-bust-inflight-243.js, test-channel-proposals.js, test-channels-client-state-152.js. No cmd/**, no CSS/HTML, no workflow, no fixture. git diff 59e398cd 572fa5af is only origin/master's advance past the merge commit (cmd/ingestor, cmd/server, config.example.json, docs/client-rx-coverage.md) — none of them PR files, so the merge commit carries no smuggled edit.

4. Tests green [T]. Listed below; test-channel-proposals-e2e.js 26/0, the channel E2Es all pass, test-app-api-bust-inflight-243.js 7/0 and test-channels-client-state-152.js 70/0.

Red before / green after [T]. On a tree of origin/master sources with only the PR's three test files copied in: test-app-api-bust-inflight-243.js 3 passed / 4 failed (exactly the three self-labelled guards pass); test-channels-client-state-152.js 68 / 2 failed — both in-flight-approval cases, with the normal-case guard green as designed; test-channel-proposals.js 40 / 0 (see N4). Each acceptance criterion in the issue therefore has a test that is red before and green after, with the one documented exception.

No behaviour change beyond the purpose [A]. The one deliberate ripple is disclosed in the PR body and holds up: observers.js's manual refresh (observers.js:184 click, :221 Enter/Space) passes bust: true, so a second manual refresh during an in-flight auto-refresh now fetches instead of joining it. That is what a manual refresh asks for, and the Kpa-clawbot#1563 request-id guard already drops the loser; the observers unit tests and both observer E2Es are green. refreshMessages()'s bust: !!opts.forceNoCache (channels.js:2658) is a different endpoint (/channels/{hash}/messages) and unaffected. Callers without bust are byte-for-byte unchanged in behaviour: the in-flight slot is always their own promise, so both new ownership checks are tautologically true for them.

I also checked the two ownership checks for leaks and ordering [A]: whichever order the superseded and superseding requests settle in, exactly one _inflight entry is written and exactly one removed, the superseded answer never reaches _apiCache, and the superseded caller still receives its own answer. The promise self-reference inside the async IIFE is out of TDZ by the time it is read (it is only read after await fetch/await res.json()).

Tests I ran [T]

All against the resolved merge tree (572fa5af = merge-tree origin/master 59e398cd, with origin/master at 2e8deaed), binaries built from it.

Command	Result
`node test-app-api-bust-inflight-243.js`	7 / 0
`node test-channels-client-state-152.js`	70 / 0
`node test-channel-proposals.js`	40 / 0
`node test-frontend-helpers.js`	707 / 0
`sh test-all.sh`	220 / 220 files
`cd cmd/server && go test ./...`	ok, 38.5 s
`cd cmd/ingestor && go test ./...`	ok, 104.0 s
`scripts/check-xss-sinks.sh --diff origin/master`	clean (exit 0), run in a scratch worktree of the merged tree; also clean in whole-file `--file` mode on both changed files

go test ./... from the repo root is not runnable — the repo is multi-module with no root module (directory prefix . does not contain main module), so the per-module invocations above are the equivalent.

E2Es against a local Go server built from the merged tree, port 13800+, e2e-fixture.db prepared exactly as the Playwright job does (freshen, the Kpa-clawbot#1486/Kpa-clawbot#1791 seed, corescope-migrate, the Kpa-clawbot#2073, #199 and #245 seeds); server stopped afterwards by port lookup:

test-channels-client-state-152-e2e.js 6/0 · test-channel-issue-1087-e2e.js 3/0 · test-channel-issue-1111-e2e.js 2/0 · test-channels-154-155-e2e.js 8/0 · test-channels-list-render-e2e.js 8/0 · test-channels-selection-flow-e2e.js 8/0 · test-channels-add-modal-e2e.js 8/0 · test-channel-fluid-e2e.js 15/0 · test-channel-decrypt-e2e.js 15/0 · test-channel-modal-e2e.js 15/0 · test-channels-ws-batch-e2e.js 6/0 · test-channels-ws-race-1498-e2e.js 5/0 · test-channels-share-color-e2e.js 6/0 · test-channel-qr-e2e.js 11/0 · test-channel-color-picker-e2e.js 10/0 · test-issue-1639-observers-sort-e2e.js all pass · test-observer-iata-1188-e2e.js all pass.

Self-starting (own server + ingestor, OS-assigned ports): test-channel-proposals-e2e.js 26/0 · test-channels-client-state-152-decrypt-e2e.js 18/0.

Mutants I ran [T]

Ten, each applied to the resolved merge tree and reverted afterwards. Eight die.

#	Mutant	Verdict
M1	`app.js`: `if (_inflight.has(inflightKey))` — `bust` ignored for the join	dead — api 4 ✗, channels 2 ✗
M2	`channels.js`: `} else if (isFirstObservation) {` — `hiddenChannels` guard dropped	dead — channels 1 ✗ (the #251 hidden-row case)
M3	`channels.js`: #225 `requestId !== channelsRequestId` check dropped on the success path	dead — channels 4 ✗ (3× #154, 1× #243)
M4	`app.js`: `} else if (ttl > 0) {` — cache-write ownership check dropped	dead — api 1 ✗, channels 1 ✗
M5	`app.js`: unconditional `_inflight.delete()` in `.finally()`	dead — api 1 ✗
M6	`channels.js`: `refreshChannelList()` → `loadChannels(true)`, bust dropped	dead — channels 2 ✗
M7	`channels.js`: `loadChannels()` passes `false` instead of `opts.bust`	dead — channels 2 ✗
M8	`channels.js`: `onRevoked` re-split into its own non-busting refresh	survives → N1 (near-equivalent; M6 covers the shared implementation)
M9	`channels.js`: `invalidateApiCache('/channels')` dropped from `refreshChannelList()`	survives, and survives the equivalent mutation on `origin/master` → N2, pre-existing
M10	`channel-proposals.js`: `state.suggestPoller.cancel()` dropped from `unmount()`	dead — proposals 1 ✗

Guardrails [T]

cmd/server untouched — no cmd/** path in the PR diff. No new map[string]interface{} anywhere in the diff.
No CSS or HTML files changed and no colour literals in added lines (the #243 hits in a naive grep are issue references).
scripts/check-xss-sinks.sh --diff origin/master: clean.
Fork guards unchanged from master: 9 occurrences of the upstream-repository guard in deploy.yml (8 conditions + 1 in the explanatory comment) and 1 in release-fast-path.yml. No workflow file is touched by this PR.
No closing keywords in any of the three commit messages, nor in the PR body ("Relates to fix(app): a requested refresh can coalesce onto an older in-flight api() request (invalidateApiCache does not clear _inflight) #243").
Author and committer on all three commits: dborup <kontakt@meshview.dk>.
git ls-remote on the branch before and after the review: 59e398cd both times.

CI, checked per job [T]

Run 37360022795, head 59e398cd: Go Build & Test success (22m27s) · Playwright E2E Tests success (24m33s, first attempt — neither the #256 Hash-Stats-sort nor the #267 backfill write-hold flake fired) · Build & Publish Docker Image success · Release Artifacts, Deploy Staging, Publish Badges all skipped (PR event). It is the only run on this head.

Not verified

The race was not reproduced on a live instance — only with controlled fetch timing in the unit tests, matching the author's own disclosure.
I did not run the full test-e2e-playwright.js battery, nor the a11y/nav/map/packets E2Es; I ran the channel and observer E2Es plus the PR's own. [K] the author reports 132/135 with 3 built-in skips.
I served public directly rather than CI's public-instrumented, so the coverage-instrumentation path in scripts/instrument-frontend.sh was not exercised.
No staging or production access, no server API key used, nothing pushed and no issue filed — read-only throughout, work done in scratch copies, the scratch worktree removed and the primary checkout left untouched.

dborup and others added 2 commits October 5, 2026 14:26

Merge remote-tracking branch 'origin/master' into codex/issue-243-api…

59e398c

…-inflight-refresh # Conflicts: # public/channels.js

dborup marked this pull request as ready for review October 5, 2026 20:29

dborup merged commit f91339f into master Oct 5, 2026
6 checks passed

dborup-agent mentioned this pull request Oct 5, 2026

fix(ui): column widths ignore colspan rows and empty first renders (#258) #268

Merged

dborup mentioned this pull request Oct 6, 2026

fix(nodes): bust in-flight /nodes on advert refresh (#279) #306

Merged

Navigation Menu

Repository navigation

fix(app): let an explicit refresh bypass an in-flight api() request (#243) - #263

fix(app): let an explicit refresh bypass an in-flight api() request (#243)#263
dborup merged 3 commits into
masterfrom
codex/issue-243-api-inflight-refresh

dborup-agent commented Oct 5, 2026 •

edited by dborup

Loading

Uh oh!

dborup-agent commented Oct 5, 2026 •

edited

Loading

Uh oh!

dborup-agent commented Oct 5, 2026

Uh oh!

dborup commented Oct 5, 2026

Uh oh!

dborup commented Oct 5, 2026

Uh oh!

adminopenclaw8-sketch commented Oct 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

dborup-agent commented Oct 5, 2026 • edited by dborup Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Other api() callers

Normal case

Overlap with #257

Tests

Mutants

Local runs (head 601bd000)

Perf

Not verified

Uh oh!

dborup-agent commented Oct 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rapport — CS-pve-agent2 PR#263 #243 — head 601bd00

Requirements

Local runs [T]

CI (run 37325629702)

Remaining

Uh oh!

dborup-agent commented Oct 5, 2026

CI update — CS-pve-agent2 PR#263 #243 — head 601bd00

Uh oh!

dborup commented Oct 5, 2026

Review — CS-MacBook PR#263 — head 601bd00

Finding table

Point-by-point

Tests and mutants run this session

Not verified

Uh oh!

dborup commented Oct 5, 2026

Rapport — CS-MacBook PR#263 runde 2 — head 59e398c

Review feedback addressed (commit 59e398cd)

Conflict resolution, hunk by hunk

Tests [T]

Mutants [T]

CI (run 37360022795, head 59e398cd)

Not done / out of scope

Uh oh!

adminopenclaw8-sketch commented Oct 5, 2026

Review — CS-Macmini PR#263 — head 59e398c

Findings

Answers to the review points

Tests I ran [T]

Mutants I ran [T]

Guardrails [T]

CI, checked per job [T]

Not verified

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dborup-agent commented Oct 5, 2026 •

edited by dborup

Loading

Other `api()` callers

Local runs (head `601bd000`)

dborup-agent commented Oct 5, 2026 •

edited

Loading

Rapport — CS-pve-agent2 PR#263 #243 — head `601bd00`

CI update — CS-pve-agent2 PR#263 #243 — head `601bd00`

Review — CS-MacBook PR#263 — head `601bd00`

Rapport — CS-MacBook PR#263 runde 2 — head `59e398c`

Review feedback addressed (commit `59e398cd`)

CI (run 37360022795, head `59e398cd`)

Review — CS-Macmini PR#263 — head `59e398c`