From 3fce33c83b58acf4b6eefc9c4e801eddae5dea57 Mon Sep 17 00:00:00 2001 From: jesse23 Date: Thu, 26 Mar 2026 21:33:37 -0400 Subject: [PATCH 1/2] chore: remove outdated pricing storage ADR document --- .../015.ordering-system.pricing-storage.md | 90 ------------------- 1 file changed, 90 deletions(-) delete mode 100644 docs/adrs/015.ordering-system.pricing-storage.md diff --git a/docs/adrs/015.ordering-system.pricing-storage.md b/docs/adrs/015.ordering-system.pricing-storage.md deleted file mode 100644 index 48bc6ad..0000000 --- a/docs/adrs/015.ordering-system.pricing-storage.md +++ /dev/null @@ -1,90 +0,0 @@ ---- -status: accepted -date: 2026-03-26 -decision-makers: Product Engineering Team, Platform Team -consulted: Product Team, Client Application Teams -informed: Engineering Leadership, SRE/Operations Team ---- - -# Pricing Storage for Ordering System - -## Context and Problem Statement - -The Ordering System serves multiple clients that require accurate, real-time pricing information. Pricing is updated frequently by the Product team and must propagate to all clients immediately. We need a centralized pricing store that is consistent, highly available, low-latency, and scalable. - -## Decision Drivers - -* Strong consistency — all clients must see the same price at all times -* Real-time propagation — changes must reach clients with no observable delay -* High availability — no single point of failure -* Low-latency reads — sub-100ms for real-time ordering workflows -* Durability and scalability as client traffic grows - -## Considered Options - -* PostgreSQL with Redis Cache -* PostgreSQL Only -* Elasticsearch with PostgreSQL -* AWS DynamoDB - -## Decision Outcome - -Chosen option: **PostgreSQL with Redis Cache**, because it is the only option that meets all three critical requirements together: strong consistency (PostgreSQL ACID), real-time propagation (LISTEN/NOTIFY + cache invalidation), and low-latency reads (Redis sub-10ms) — without vendor lock-in. - -### Consequences - -* Good, because ACID guarantees ensure consistent, durable pricing data across all clients -* Good, because LISTEN/NOTIFY enables real-time propagation without a separate message broker -* Good, because Redis cache delivers sub-10ms reads, well within the 100ms SLA -* Bad, because two systems increase operational complexity -* Bad, because cache invalidation requires careful implementation to avoid serving stale prices - -### Confirmation - -* Integration tests: write a price update, verify all client read paths return the new value within SLA -* Load tests: p99 read latency must stay below 100ms at peak traffic -* Production alert: Redis cache hit ratio below 80% or cache staleness above threshold triggers PagerDuty - -## Pros and Cons of the Options - -### PostgreSQL with Redis Cache - -Write-through to PostgreSQL; Redis as cache. LISTEN/NOTIFY broadcasts changes; clients invalidate and re-fetch. - -* Good, because ACID consistency and durability -* Good, because sub-10ms cache reads meet latency SLA -* Good, because LISTEN/NOTIFY propagates changes in real time without extra infrastructure -* Bad, because two systems to operate and monitor -* Bad, because cache invalidation bugs can cause stale price reads - -### PostgreSQL Only - -All reads and writes go directly to PostgreSQL. - -* Good, because simple — one system, no cache consistency issues -* Bad, because read latency (5–50ms) may breach SLA under peak load -* Bad, because becomes a read bottleneck as client count grows; read replicas introduce replication lag - -### Elasticsearch with PostgreSQL - -PostgreSQL as source of truth; Elasticsearch indexed via background sync for reads. - -* Good, because fast and scalable reads for complex queries -* Bad, because background sync means updates are not real-time — violates the propagation requirement -* Bad, because significant operational overhead for a simple key-value pricing lookup pattern - -### AWS DynamoDB - -DynamoDB as primary store with Streams for change propagation. - -* Good, because fully managed with built-in HA and auto-scaling -* Bad, because eventual consistency by default; strong consistency costs more -* Bad, because vendor lock-in and unpredictable cost at scale - -## More Information - -Change propagation flow: write to PostgreSQL → invalidate Redis key → publish on `pricing_updates` LISTEN/NOTIFY channel → clients refresh from cache. - -Key risks: cache stampede (mitigate with short TTLs + request coalescing), stale data on invalidation failure (mitigate with 5–10 min TTL safety net), Redis node loss (mitigate with Redis Cluster; cache rebuilds from PostgreSQL). - -Revisit if the system expands to 10+ regions or pricing model complexity requires full-text search. From e9ada56216c523c1cd0563fabcb4bb9714fe2aa4 Mon Sep 17 00:00:00 2001 From: jesse23 Date: Thu, 26 Mar 2026 21:47:08 -0400 Subject: [PATCH 2/2] fix: prevent ghost cursor on DECSCUSR style change (#19) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ghostty-web's render loop only clears the cursor row when the cursor moves or cursorBlink=true. Switching vim normal→insert (steady block→bar) satisfies neither: cursor stays put, both modes non-blinking. The old block cursor shape persisted on canvas; the new bar was drawn on top, leaving the cursor visually unchanged. Force a full repaint (forceAll=true) in applyDecscusr when the net cursor style/blink differs from the initial values at function entry. Capturing initial vs final (rather than a mid-loop changed flag) avoids a spurious repaint when multiple sequences in one chunk cancel each other out. ADR 013 manual verification passed because vim's full-screen redraw on open makes all rows dirty, clearing the cursor correctly. The gap only shows on subsequent in-place mode switches. Adds ADR 015 documenting the ghost cursor root cause and the fix. Co-authored-by: Sisyphus --- docs/adrs/015.client.cursor-ghost-repaint.md | 116 +++++++++++++++++++ src/client/cursor.test.ts | 58 +++++++++- src/client/cursor.ts | 17 +++ 3 files changed, 190 insertions(+), 1 deletion(-) create mode 100644 docs/adrs/015.client.cursor-ghost-repaint.md diff --git a/docs/adrs/015.client.cursor-ghost-repaint.md b/docs/adrs/015.client.cursor-ghost-repaint.md new file mode 100644 index 0000000..737c669 --- /dev/null +++ b/docs/adrs/015.client.cursor-ghost-repaint.md @@ -0,0 +1,116 @@ +# ADR 015: Client — Force repaint on DECSCUSR to prevent ghost cursor + +**SPEC:** [client](../specs/client.md) +**Status:** Accepted +**Date:** 2026-03-26 +**Supersedes:** Partially extends [ADR 013](013.client.cursor-style.md) + +--- + +## Context + +ADR 013 introduced `cursor.ts` to intercept DECSCUSR sequences and update +`term.options.cursorStyle` / `term.options.cursorBlink` via ghostty-web's +options Proxy. The approach was sound, but it addressed only the "what to set" +layer. A second rendering gap remained, causing cursor shape changes to be +visually invisible in practice. + +### The ghost cursor problem + +ghostty-web's `CanvasRenderer.render()` only **clears the cursor row** (erases +the old cursor shape from canvas) under two conditions: + +```js +// ghostty-web/dist/ghostty-web.js — CanvasRenderer.render() +const s = cursor.x !== lastPos.x || cursor.y !== lastPos.y; +if (s || this.cursorBlink) { + renderLine(cursor_row); // ← erases old cursor shape +} +// always runs after: +renderCursor(cursor.x, cursor.y); // ← draws new cursor shape +``` + +When vim switches normal → insert mode (steady block `\x1b[2 q` → steady bar +`\x1b[6 q`), neither condition is true: + +- **Cursor did not move** — pressing `i` leaves the cursor in place (`s = false`) +- **`cursorBlink = false`** — both normal and insert use steady cursors + +Result: the old block cursor background stays on canvas and the new bar cursor +is drawn on top of it, leaving the cursor visually unchanged. + +### Why ADR 013 manual verification passed + +When vim first opens it performs a full-screen redraw, writing to every row +including the cursor row. The dirty-row paint path also calls +`renderLine(cursor_row)`, which clears and redraws the cursor correctly via +`renderCursor()`. The first mode switch on vim open therefore works fine. + +The failure only surfaces on **subsequent in-place mode switches** (pressing `i` +or `Esc` without moving the cursor), where only the status line is updated and +the cursor row stays clean. This scenario was not covered by the original manual +verification. + +--- + +## Decision + +After detecting a cursor style or blink change, call +`term.renderer.render(term.wasmTerm, true, term.viewportY)` synchronously. + +`forceAll = true` bypasses the dirty-state and blink checks, repainting every +row. This clears the stale cursor shape from the cursor row before +`renderCursor()` draws the new shape at the end of the same render pass. + +`term.renderer` and `term.wasmTerm` are public properties on `Terminal` +(declared without `private` in ghostty-web's `.d.ts`). `GhosttyTerminal` +satisfies both `IRenderable` and `IScrollbackProvider` structurally, so no +casting is needed. + +The force repaint is guarded by a `changed` flag and only fires when style or +blink actually differs from the current value — i.e., on vim mode switches, +which are infrequent. There is no per-message overhead. + +--- + +## Considered Options + +**Option A: `requestAnimationFrame` toggle on `cursorBlink`** + +Temporarily set `cursorBlink = true` for one RAF frame so the render loop's +`if (cursorBlink)` branch clears the cursor row, then restore the correct value +via RAF callback. Correct but adds async complexity and a one-frame blink pulse. + +**Option B: Ignore `cursorBlink` updates entirely** + +Only update `cursorStyle` from DECSCUSR; leave `cursorBlink` controlled by +config. With the default `cursorStyleBlink: true`, blinking ensures the cursor +row is always cleared. Fragile: silently breaks for users who disable blinking +in config. + +**Option C: Force repaint via `term.renderer.render(forceAll=true)` (chosen)** + +Synchronous, no async, no side effects on blink state, works regardless of +config. The extra full-canvas render fires only on mode switches and costs ~1 ms +on modern hardware. + +--- + +## Consequences + +- vim, neovim, fish — cursor shape changes are immediately visible on all mode + switches regardless of blink config or cursor position. +- One additional full canvas render per DECSCUSR style change. No per-message + cost; normal typing is unaffected. +- If ghostty-web adds a public `refresh()` or `invalidateCursorRow()` API, the + `forceAll` call can be replaced with the narrower method and this ADR updated. +- When ghostty-web implements DECSCUSR natively (reading `cursor_visual_style` + from the WASM render state — see ADR 013's upstream fix checklist), the entire + `cursor.ts` module and this workaround are removed together. + +## Related Decisions + +- [ADR 013 — DECSCUSR cursor style via PTY intercept](013.client.cursor-style.md): + introduced `cursor.ts`; this ADR closes the rendering gap it left open. +- [ADR 010 — Client UX polish](010.client.ux-polish.md): established the + WebSocket message handling that the intercept hooks into. diff --git a/src/client/cursor.test.ts b/src/client/cursor.test.ts index 37fa3c5..6117f5b 100644 --- a/src/client/cursor.test.ts +++ b/src/client/cursor.test.ts @@ -1,10 +1,24 @@ -import { describe, expect, test } from 'bun:test'; +import { describe, expect, mock, test } from 'bun:test'; import { applyDecscusr } from './cursor'; function makeTerm(): { options: { cursorStyle: string; cursorBlink: boolean } } { return { options: { cursorStyle: 'block', cursorBlink: false } }; } +function makeTermWithRenderer(): { + options: { cursorStyle: string; cursorBlink: boolean }; + renderer: { render: ReturnType }; + wasmTerm: object; + viewportY: number; +} { + return { + options: { cursorStyle: 'block', cursorBlink: false }, + renderer: { render: mock(() => {}) }, + wasmTerm: {}, + viewportY: 0, + }; +} + describe('applyDecscusr', () => { test('Ps 0 — default reset — sets block blinking', () => { const term = makeTerm(); @@ -92,3 +106,45 @@ describe('applyDecscusr', () => { expect(term.options.cursorBlink).toBe(false); }); }); + +describe('applyDecscusr — force repaint', () => { + test('calls renderer.render with forceAll=true when style changes', () => { + const term = makeTermWithRenderer(); + applyDecscusr(term as never, '\x1b[6 q'); + expect(term.renderer.render).toHaveBeenCalledTimes(1); + expect(term.renderer.render).toHaveBeenCalledWith(term.wasmTerm, true, 0); + }); + + test('calls renderer.render when only blink changes', () => { + const term = makeTermWithRenderer(); + term.options.cursorBlink = true; + applyDecscusr(term as never, '\x1b[2 q'); + expect(term.renderer.render).toHaveBeenCalledTimes(1); + }); + + test('does not call renderer.render when style is unchanged', () => { + const term = makeTermWithRenderer(); + term.options.cursorStyle = 'block'; + term.options.cursorBlink = false; + applyDecscusr(term as never, '\x1b[2 q'); + expect(term.renderer.render).not.toHaveBeenCalled(); + }); + + test('does not call renderer.render when no DECSCUSR sequence', () => { + const term = makeTermWithRenderer(); + applyDecscusr(term as never, 'hello world'); + expect(term.renderer.render).not.toHaveBeenCalled(); + }); + + test('calls renderer.render once even with multiple changing sequences', () => { + const term = makeTermWithRenderer(); + applyDecscusr(term as never, '\x1b[2 q\x1b[6 q'); + expect(term.renderer.render).toHaveBeenCalledTimes(1); + }); + + test('does not call renderer.render when sequences cancel out to original values', () => { + const term = makeTermWithRenderer(); + applyDecscusr(term as never, '\x1b[6 q\x1b[2 q'); + expect(term.renderer.render).not.toHaveBeenCalled(); + }); +}); diff --git a/src/client/cursor.ts b/src/client/cursor.ts index d19e752..7d68c2d 100644 --- a/src/client/cursor.ts +++ b/src/client/cursor.ts @@ -21,6 +21,8 @@ const ESC = '\x1b'; const DECSCUSR = new RegExp(`${ESC}\\[(\\d*) q`, 'g'); export function applyDecscusr(term: Terminal, data: string): void { + const initialStyle = term.options.cursorStyle; + const initialBlink = term.options.cursorBlink; DECSCUSR.lastIndex = 0; let match = DECSCUSR.exec(data); while (match !== null) { @@ -32,4 +34,19 @@ export function applyDecscusr(term: Terminal, data: string): void { } match = DECSCUSR.exec(data); } + // ghostty-web's render loop only clears the cursor row when the cursor moves or + // cursorBlink is true. When switching to a non-blinking style (e.g. block→bar in + // vim normal→insert), the old cursor shape stays painted on canvas and the new + // shape is drawn on top — leaving a ghost of the previous cursor. + // + // Force a full repaint so the cursor row is cleared before the new shape is drawn. + // term.renderer and term.wasmTerm are public on Terminal; forceAll=true repaints + // every row, which clears the stale cursor, and renderCursor() draws the new shape. + if ( + (term.options.cursorStyle !== initialStyle || term.options.cursorBlink !== initialBlink) && + term.renderer && + term.wasmTerm + ) { + term.renderer.render(term.wasmTerm, true, term.viewportY); + } }