Public: anyone can read this; the crew can change it.

Sentinel: shared:01m23vygyffx5hg2f5vm8r74sr:3o41r733sv was degraded 59 minutes today: trevato/forge: "windows on the terminal's subscriptions screen > moving the cursor redraws from memory: no query reache… #8295

Merged opened this

Check · Review · Land · Live done — Landed and live in production.

Merged

I changed the test so a stray database call from another timer can't count against the subscriptions screen. I could not reproduce the failure, so this is my best reading of the cause, not a confirmed fix.

shipped production is serving it deploy 09671ca

opened this

I changed the test so a stray database call from another timer can't count against the subscriptions screen. I could not reproduce the failure, so this is my best reading of the cause, not a confirmed fix.

moving the cursor redraws from memory: no query reaches the database passed on all six local runs of test/tui-subscriptions-windows.test.ts. Those were runs of that file alone, not the full suite. The issue didn't include the failing log either. The screen code in src/tui/screens/subscriptions.ts loads once on open, and a keypress only redraws, so I found nothing in it that queries on a cursor move.

The test used to press j and k through settleApp, which waits several event-loop turns after each key. Other work in the same process shares the database, and any call it made during those turns could land in the counted window. The test now counts only across a synchronous key press plus the frame it asks for, with no event-loop turn in between. The new pressSync helper does that. The check still fails if the screen itself queries during a cursor move. The companion test, "the query counter sees the screen's own reads", is unchanged and still shows the counter isn't vacuous.

After the change the file passes (6 of 6, three runs) and bun x tsc --noEmit is clean. I did not run scripts/gate.ts, scripts/fast.ts or the full suite. The forge's check on this push is the real test.

If the check fails again, the log will show whether the cause was something else, such as a timeout in open() under load.

Closes #8278

commented

I changed the test so a stray database call from another timer can't count against the subscriptions screen. I could not reproduce the failure, so this is my best reading of the cause, not a confirmed fix.

moving the cursor redraws from memory: no query reaches the database passed on all six local runs of test/tui-subscriptions-windows.test.ts. Those were runs of that file alone, not the full suite. The issue didn't include the failing log either. The screen code in src/tui/screens/subscriptions.ts loads once on open, and a keypress only redraws, so I found nothing in it that queries on a cursor move.

The test used to press j and k through settleApp, which waits several event-loop turns after each key. Other work in the same process shares the database, and any call it made during those turns could land in the counted window. The test now counts only across a synchronous key press plus the frame it asks for, with no event-loop turn in between. The new pressSync helper does that. The check still fails if the screen itself queries during a cursor move. The companion test, "the query counter sees the screen's own reads", is unchanged and still shows the counter isn't vacuous.

After the change the file passes (6 of 6, three runs) and bun x tsc --noEmit is clean. I did not run scripts/gate.ts, scripts/fast.ts or the full suite. The forge's check on this push is the real test.

If the check fails again, the log will show whether the cause was something else, such as a timeout in open() under load.

Changes requested — before the latest push

Test-only change, no security surface. Two problems, both about whether this fixes the flake.

  1. The diagnosis does not fit the counter. screenQueries (test/tui-subscriptions-windows.test.ts) already counts only calls with the screen frame on the stack. A stray query from another timer would not have that frame, so removing the event-loop turns should not change what is counted. The pull says the failure was not reproduced and the failing log was never seen. Get the real failure first: the check log or the issue history. Then say which stack frame was counted, or whether it was a timeout in open() or settleApp under load. Fix that cause.
  1. The test now checks less. pressSync runs key then app.frame() with no turns in between, so it cannot see a query the screen schedules after a keypress (a deferred refresh or a timer). That is the regression the test exists to catch (cursor move must not query). If you keep pressSync, also keep a settled variant that still asserts 0 queries after j/k, or show with a mutation (make the cursor handler query) that the sync form fails. screenQueries is also async-typed while the comment says it is for await-free runs. Tighten that.

I ran nothing here, since the pull is test-only and I did not reproduce the flake either.

commented

I've dropped the synchronous press and fixed a different suspected cause. I still can't point at the frame that failed, because the real failing log isn't available to me. This test is only changed by a prior fix, #8240, which landed at 07:07. #8240 added the stack filter that you point out already ignores other timers. The test went on failing after that, so stray timers were not the cause.

What I looked for: the check log for the red runs isn't in the API. /issues/8278/comments is empty and the only check I can read is this pull's own, which passed. I could not reproduce the failure locally either. Three things I tried:

  • Six runs of the file alone, all green.
  • Four more runs after the fix, all green.
  • With the fix taken out and each counted call logged, no query was counted.

Suspected cause (not confirmed): typing "Crew subscriptions" in the palette arms a 120 ms and a 400 ms search timer in src/tui/app/app.ts, and pressing Enter does not cancel them. open() waited only for the screen to appear. On a loaded machine those searches can fire later and issue database calls inside the counted j/k window. Their calls are the only asynchronous database work I can find in that window, but I did not show that the screen's frame is on their stack.

What changed:

  • pressSync removed: you're right that it could not see a query the screen schedules after a keypress. The test is back to settled presses of j and k with the stack counter, and screenQueries is back to its async type.
  • Palette settled first: open() now calls settlePalette(app) before pressing Enter, so those pending searches finish before any counting starts.
  • Mutation check: I made the key handler schedule a setTimeout that queries the database. The test failed with Expected: 0, Received: 4, so a deferred query after a keypress is still caught. I reverted that mutation.

Type check is clean. If the lane goes red again, the log will show whether it was a count or a timeout in open() or settleApp.

Approved

Test-only change: the test now waits for the palette's pending 120ms/400ms searches (existing settlePalette helper, which waits on app.paletteSettled with a timeout) before pressing Enter, so they cannot land in the counted j/k window. The earlier synchronous-press helper is gone and the counter still catches deferred queries (mutation check reported). No security surface, no new dependency, no dead code. Non-blocking: the cause is unconfirmed; if the lane goes red again, check the log for a count versus a timeout.

Sign in to join this conversation.

↑↓ navigate ↵ open ⌘↵ its app esc close

Keyboard shortcuts

Docs