diff --git a/.claude/skills/address-review/SKILL.md b/.claude/skills/address-review/SKILL.md index ed4c00d..c58c407 100644 --- a/.claude/skills/address-review/SKILL.md +++ b/.claude/skills/address-review/SKILL.md @@ -23,11 +23,17 @@ That variable is the whole of the addressing. It names **this tab's** review, so there is no repository to pass and no way to address comments meant for another worktree. +Give every call a deadline — `--max-time 30`. The server runs `git` for some of +these, and a repository being built in another pane can make that slow; a curl +with no deadline turns that into a tool timeout that says nothing about what +went wrong. If one does trip, say so and try once more rather than treating it +as a missing review: the answer was late, not absent. + If it is empty — you are running outside playpen, or in a shell started before the server came up — discover it instead: ```bash -curl -s http://127.0.0.1:8420/api/tabs +curl -s --max-time 30 http://127.0.0.1:8420/api/tabs ``` That lists every tab with the `path` it is reviewing. Match `path` against @@ -40,7 +46,7 @@ stop. Confirm the review is open: ```bash -curl -s "$BASE/api/repo" +curl -s --max-time 30 "$BASE/api/repo" ``` - `{"open":true,…}` — good, go on. @@ -50,7 +56,7 @@ curl -s "$BASE/api/repo" ## 2. Fetch the pending comments ```bash -curl -s "$BASE/api/review/pending" +curl -s --max-time 30 "$BASE/api/review/pending" ``` Returns this review's submitted, unresolved comments. Each has: @@ -88,7 +94,7 @@ For every pending comment, in order: 3. **Post an inline reply** describing exactly what you did (or your answer): ```bash - curl -s -X POST "$BASE/api/comments//replies" \ + curl -s --max-time 30 -X POST "$BASE/api/comments//replies" \ -H 'Content-Type: application/json' \ -d '{"body":"Done — main() now logs and returns the error instead of printing.","author":"claude"}' ``` @@ -96,7 +102,7 @@ For every pending comment, in order: 4. **Resolve the thread** once it's fully handled (skip if you asked a question): ```bash - curl -s -X POST "$BASE/api/comments//resolve" + curl -s --max-time 30 -X POST "$BASE/api/comments//resolve" ``` Replies and resolutions appear in the review pane immediately over its live diff --git a/.claude/skills/leave-review/SKILL.md b/.claude/skills/leave-review/SKILL.md index f02d659..dd6572a 100644 --- a/.claude/skills/leave-review/SKILL.md +++ b/.claude/skills/leave-review/SKILL.md @@ -21,19 +21,26 @@ job, and your comments land in its queue automatically (see step 7). ```bash BASE="$PLAYPEN_REVIEW_URL" # e.g. http://127.0.0.1:8420/t/tab3 -curl -s "$BASE/api/repo" +curl -s --max-time 30 "$BASE/api/repo" ``` `$PLAYPEN_REVIEW_URL` is exported into every terminal pane and names **this tab's** review, so there is nothing to choose and no way to leave your review on someone else's branch. +Give every call a deadline — `--max-time 30`. The server runs `git` for some of +these, and a repository being built in another pane can make that slow; a curl +with no deadline turns that into a tool timeout that says nothing about what +went wrong. If one does trip, say so and try once more rather than treating it +as a missing review: the answer was late, not absent. + - `{"open":true,…}` with a `context` object — good, go to step 2. - `{"open":false}` or a 409 — this tab has no review pane. Tell the user to open one (**Ctrl+Shift+D**) and stop. Don't review a different tab. - `$PLAYPEN_REVIEW_URL` empty, or connection refused — list the tabs with - `curl -s http://127.0.0.1:8420/api/tabs` and match a tab's `path` against - `git rev-parse --show-toplevel`. If nothing matches, stop and say so. + `curl -s --max-time 30 http://127.0.0.1:8420/api/tabs` and match a tab's + `path` against `git rev-parse --show-toplevel`. If nothing matches, stop and + say so. ## 2. Find out which diff to review @@ -59,7 +66,7 @@ whose line numbers the pane can place a comment on. ## 3. Get the diff ```bash -curl -s "$BASE/api/diff?base=main&uncommitted=true" +curl -s --max-time 30 "$BASE/api/diff?base=main&uncommitted=true" ``` The `patch` field is the exact bytes the pane renders, and `files` is the @@ -163,7 +170,7 @@ Three details that decide whether a comment lands where you meant: **Don't repeat what's already been said.** Fetch the existing threads first: ```bash -curl -s "$BASE/api/comments" +curl -s --max-time 30 "$BASE/api/comments" ``` Skip anything the user already raised, and anything **you** raised on an earlier @@ -192,7 +199,7 @@ Write each body like a comment in a thread someone has to read: ## 6. Post them ```bash -curl -s -X POST "$BASE/api/comments" -H 'Content-Type: application/json' -d '{ +curl -s --max-time 30 -X POST "$BASE/api/comments" -H 'Content-Type: application/json' -d '{ "level":"line","file":"src/review/Store.zig","side":"new","line":84,"endLine":91, "author":"claude", "body":"Should fix: save() runs while the write lock is held, so a slow disk blocks every reader for the length of the write. Snapshot the list under the lock and write outside it." diff --git a/README.md b/README.md index a8d15a7..93b5125 100644 --- a/README.md +++ b/README.md @@ -897,7 +897,7 @@ The endpoints, all under `/t//api`: | `GET repo` | the repository, its refs, the comment counts, and the diff selection on screen | | `POST repo/context` | what the page publishes when you change the base ref | | `GET diff` | `base`, `uncommitted`, `commit`, `force`, `ignoreWhitespace` | -| `GET diff/revision` | a digest of what that same selection resolves to now — one hash, so the page can poll it | +| `GET diff/revision` | a digest of what that same selection resolves to now — one hash, so the page can poll it; 503 while git is busy with work someone is waiting on | | `GET file` | a file's contents at a ref, for expanding collapsed context | | `GET/POST comments` | list, or open a thread | | `PATCH/DELETE comments/{id}` | edit or delete one | @@ -923,12 +923,31 @@ itself is not that answer: it would lose your scroll position, your place in a hunk, and whatever you had half-typed into a composer. So the page asks `GET diff/revision` every few seconds — a hash of the patch plus -`HEAD`, which is cheap enough to ask for on a timer and catches both an -uncommitted edit and work being committed out from under the range. When it stops -matching the revision the diff came with, a banner says so and offers the refresh. -Dismissing it keeps the diff you are reading and re-arms against what is there -now, so the *next* change tells you too. Polling stops while the pane is hidden, -and starts again the moment it comes back. +`HEAD`, which catches both an uncommitted edit and work being committed out from +under the range. When it stops matching the revision the diff came with, a banner +says so and offers the refresh. Dismissing it keeps the diff you are reading and +re-arms against what is there now, so the *next* change tells you too. Polling +stops while the pane is hidden, and starts again the moment it comes back. + +The poll is a `git diff`, though, and the interesting case is a review open in +seven tabs on a repository that is being built in the terminal pane next door. +Left to a fixed interval, that is a git process running in this app most of the +time, and the agent's `GET diff` waits behind polls whose answers nobody is +reading. + +So the server sorts git into two kinds of work and only ever holds one of them +up. Anything with somebody on the other end of it — a diff being opened, the +**Refresh** you just clicked, an agent fetching the patch it is about to review — +runs the moment it arrives, every time, and is never queued behind anything. A +poll runs only while nothing is being waited on and the app is not already busy +with a few of them; otherwise it is turned away with a 503 on the spot. That +costs nothing, because a poll has no answer anybody is waiting for, and it means +a click does not merely jump the queue — it clears the field of polls for as long +as it takes. + +Each pane also spaces its own next poll by what the last one actually cost, so a +pane on a slow repository asks less often on its own, and a refused one waits +longer still. A pane on a small repository never notices any of it. Comments are the other half, and they work the other way round: those arrive over `GET events` and are applied live, because a thread appearing in the rail doesn't diff --git a/mise.toml b/mise.toml index d6131b6..a6d6ce0 100644 --- a/mise.toml +++ b/mise.toml @@ -232,7 +232,14 @@ export __EGL_VENDOR_LIBRARY_DIRS="$eglvendor" export LIBGL_DRIVERS_PATH="$dridrivers" export GBM_BACKENDS_PATH="$gbmbackends" -exec "$libexec/playpen" "\$@" +# Warnings — a git invocation that timed out, a review server that could not +# listen — are the only account of why a review pane is behaving oddly, and a +# desktop launcher gives the app no terminal to print them to. Truncated per +# launch rather than appended, so this stays a log of the session you are in +# and never grows without bound. +log="\${XDG_STATE_HOME:-\$HOME/.local/state}/playpen" +mkdir -p "\$log" +exec "$libexec/playpen" "\$@" 2>"\$log/playpen.log" EOF chmod 755 "$bindir/playpen" diff --git a/src/main.zig b/src/main.zig index 630f97d..d95d88c 100644 --- a/src/main.zig +++ b/src/main.zig @@ -6,6 +6,7 @@ //! else — process management, rendering, and the GTK4 UI — lives here. const std = @import("std"); +const builtin = @import("builtin"); const adw = @import("adw"); const gio = @import("gio"); @@ -21,10 +22,28 @@ pub const std_options: std.Options = .{ .log_level = .info, }; -var gpa: std.heap.DebugAllocator(.{}) = .init; +/// The process allocator, which everything in the app shares: the GTK main +/// loop, every terminal's parser, and every review connection thread. +/// +/// `DebugAllocator` only in a debug build, and this is not a matter of taste. +/// It is thread-safe by way of one mutex around every allocation and every +/// free, and its backing allocator is the page allocator — so a large +/// allocation is an `mmap` and its release an `munmap`, both taken under that +/// single process-wide lock. A review pane's poll allocates a whole `git diff` +/// and drops it a moment later, several times a second across a handful of open +/// reviews, and each one of those was stalling the main loop and every other +/// request behind the same lock. `c_allocator` has a per-thread cache and no +/// global lock; libc is already linked for GTK. +var debug_gpa: std.heap.DebugAllocator(.{}) = .init; + +fn gpa() std.mem.Allocator { + return if (builtin.mode == .Debug) debug_gpa.allocator() else std.heap.c_allocator; +} pub fn main() u8 { - defer _ = gpa.deinit(); + defer if (builtin.mode == .Debug) { + _ = debug_gpa.deinit(); + }; // Before the allocator's own teardown, since the settings arena comes out // of it. A no-op if the app never got as far as activating. @@ -51,7 +70,7 @@ pub fn main() u8 { fn onActivate(app: *adw.Application, _: ?*anyopaque) callconv(.c) void { // The file both of the next two read from: the scheme, and the tabs the // window opens itself with. - Settings.init(gpa.allocator()); + Settings.init(gpa()); // Before the window, so that the first frame is drawn in the scheme the // user chose rather than repainted into it a moment later. @@ -65,9 +84,9 @@ fn onActivate(app: *adw.Application, _: ?*anyopaque) callconv(.c) void { // created and every terminal is handed the endpoint of the tab it opens in. // Started unconditionally rather than on the first review pane, so an agent // running in a tab has a `PLAYPEN_REVIEW_URL` from the moment it starts. - review.init(gpa.allocator()); + review.init(gpa()); - const window = Window.create(gpa.allocator(), app) catch |err| { + const window = Window.create(gpa(), app) catch |err| { std.log.err("failed to create window: {s}", .{@errorName(err)}); return; }; diff --git a/src/review/Server.zig b/src/review/Server.zig index bc14590..2708b0f 100644 --- a/src/review/Server.zig +++ b/src/review/Server.zig @@ -69,6 +69,76 @@ const sse_heartbeat_ms = 15_000; /// reconnect anyway. const sse_backlog = 32; +/// How many git invocations may be in flight before a poll gives way. +/// +/// Every review pane polls for a moved diff on a timer, and every one of those +/// polls is a `git diff` over a whole work tree. With a review open in seven +/// tabs that is a git process starting somewhere in this app most of the time, +/// on top of whatever the terminal beside it is already doing to the same +/// repository — which is usually a build. Unbounded, they land on the machine +/// all at once and the request someone is actually waiting on finishes last. +/// +/// So polls yield and nothing else does. This is the threshold they yield at; +/// `GitLoad` is the rule itself. +const git_busy_at = 4; + +/// What git is doing right now, and who for. +/// +/// The distinction this exists to make is between a git invocation somebody is +/// waiting on — a diff being opened, a **Refresh** being clicked, an agent +/// fetching the patch it is about to review — and one that is a timer firing in +/// a page that will not look any different if it never gets an answer. +/// +/// The first kind never waits. Not for a slot, not behind a queue, not for +/// anything: a person who clicks refresh has told you exactly what they want and +/// making them wait for a poll to finish first is indefensible. It runs +/// immediately and every time. +/// +/// The second kind yields to the first. A poll runs only when nothing is being +/// waited on and the app is not already busy with `git_busy_at` of them, and +/// otherwise is turned away at once — which costs nothing, because the page +/// ignores a failed poll and asks again on its next tick. So a click does not +/// merely jump the queue; it clears the field of polls for as long as it runs. +const GitLoad = struct { + mutex: std.Io.Mutex = .init, + + /// Invocations with somebody on the other end of them. + waited_on: u32 = 0, + + /// Every invocation in flight, polls included. + total: u32 = 0, + + /// Start work someone is waiting on. Never refused, never delayed. + fn enter(self: *GitLoad, io: std.Io) void { + self.mutex.lockUncancelable(io); + defer self.mutex.unlock(io); + self.waited_on += 1; + self.total += 1; + } + + fn leave(self: *GitLoad, io: std.Io) void { + self.mutex.lockUncancelable(io); + defer self.mutex.unlock(io); + self.waited_on -= 1; + self.total -= 1; + } + + /// Start a poll, if this is a moment for one. False means it is not. + fn enterPoll(self: *GitLoad, io: std.Io) bool { + self.mutex.lockUncancelable(io); + defer self.mutex.unlock(io); + if (self.waited_on > 0 or self.total >= git_busy_at) return false; + self.total += 1; + return true; + } + + fn leavePoll(self: *GitLoad, io: std.Io) void { + self.mutex.lockUncancelable(io); + defer self.mutex.unlock(io); + self.total -= 1; + } +}; + /// Cap on a request body. Comment bodies are prose; anything larger is a mistake /// or an attack, and either way there is nothing here worth the memory. const max_body = 4 * 1024 * 1024; @@ -149,6 +219,9 @@ mutex: std.Io.Mutex = .init, tabs: std.ArrayListUnmanaged(*Tab) = .empty, clients: std.ArrayListUnmanaged(*Client) = .empty, +/// Who git is working for at the moment. See `GitLoad`. +git_load: GitLoad = .{}, + listener: ?std.Io.net.Server = null, port: u16 = 0, @@ -648,7 +721,11 @@ fn handleRepo( tab: Resolved, ) !void { const repo = tab.repo orelse return writeJson(a, request, .ok, .{ .open = false }); - const info = git.info(repo, a, self.io); + const info = blk: { + self.git_load.enter(self.io); + defer self.git_load.leave(self.io); + break :blk git.info(repo, a, self.io); + }; const store = tab.store.?; return writeJson(a, request, .ok, .{ @@ -741,7 +818,11 @@ fn handleDiff( .ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false, }; - const payload = git.diff(repo, a, self.io, ctx, opts) catch |err| return writeError( + self.git_load.enter(self.io); + const produced = git.diff(repo, a, self.io, ctx, opts); + self.git_load.leave(self.io); + + const payload = produced catch |err| return writeError( a, request, if (err == error.BadCommit) .bad_request else .bad_gateway, @@ -774,7 +855,28 @@ fn handleRevision( .ignore_whitespace = queryFlag(a, query, "ignoreWhitespace") catch false, }; - const rev = git.revision(repo, a, self.io, ctx, opts) catch |err| return writeError( + // Best-effort, unlike every other git route here: this is a timer firing in + // a page that will look no different if the answer never comes, and the + // refresh somebody just clicked in the next tab wants git more. A poll that + // arrives at a bad moment says so and is asked again on the next tick — see + // `GitLoad`, and `POLL_MS` in the page for what it does with a refusal. + if (!self.git_load.enterPoll(self.io)) return writeError( + a, + request, + .service_unavailable, + "git is busy with something someone is waiting on — poll again shortly", + ); + defer self.git_load.leavePoll(self.io); + + // The branch rides along with the digest so the page can notice that the + // work tree moved to another branch — which the digest alone cannot always + // say. The digest hashes HEAD's *sha*, so branching off the commit you are + // already on (`git switch -c`) leaves it identical while the name in the + // review's header is now wrong. Both come out of one `rev-parse`, which is + // also the sha the digest wants — see `git.head`. + const at = git.head(repo, a, self.io); + + const rev = git.revision(repo, a, self.io, ctx, opts, at.sha) catch |err| return writeError( a, request, if (err == error.BadCommit) .bad_request else .bad_gateway, @@ -783,15 +885,9 @@ fn handleRevision( else => "git could not produce that diff — check the base ref", }, ); - // The branch rides along with the digest so the page can notice that the - // work tree moved to another branch — which the digest alone cannot always - // say. `digest` hashes HEAD's *sha*, so branching off the commit you are - // already on (`git switch -c`) leaves it identical while the name in the - // review's header is now wrong. One `rev-parse` next to the `git diff` this - // endpoint already runs is not a cost worth avoiding. return writeJson(a, request, .ok, .{ .revision = rev, - .branch = git.currentBranch(repo, a, self.io), + .branch = at.branch, }); } @@ -812,7 +908,11 @@ fn handleFile( // A miss is ordinary — a newly added file has no base version — so this is a // 404 with no detail rather than something the UI has to explain. - const content = git.fileAt(repo, a, self.io, ref, path) catch return notFound(request); + self.git_load.enter(self.io); + const read = git.fileAt(repo, a, self.io, ref, path); + self.git_load.leave(self.io); + + const content = read catch return notFound(request); return request.respond(content, .{ .extra_headers = &.{.{ .name = "content-type", .value = "text/plain; charset=utf-8" }}, }); diff --git a/src/review/git.zig b/src/review/git.zig index b6cb805..3fa7100 100644 --- a/src/review/git.zig +++ b/src/review/git.zig @@ -25,6 +25,14 @@ const model = @import("model.zig"); /// forever. const timeout_s = 60; +/// The same, for the poll a review pane runs on a timer. +/// +/// Much shorter, because a poll is disposable and holds one of the app's git +/// slots while it runs: a wedged one would otherwise keep a slot from the diff +/// somebody is waiting to open for a whole minute, and the answer it eventually +/// came back with would be a minute stale anyway. +const poll_timeout_s = 15; + /// Cap on what one git invocation may print. A patch is the big one: the /// oversize guard below is what normally keeps it in hand, and this is the /// backstop for the cases the guard cannot see coming. @@ -71,6 +79,37 @@ pub fn open(gpa: std.mem.Allocator, io: std.Io, path: []const u8) Error!Repo { } /// The short name of the checked-out branch, or "HEAD" when detached. +/// What HEAD is, both ways, from a single `rev-parse`. +/// +/// The poll wants the sha (it goes into the digest) and the branch name (the +/// page notices a `git switch` with it) on every tick, and asking for them +/// separately was two process spawns where git will answer both from one — a +/// difference worth having when this runs once per review pane per tick. +/// +/// A repository with no commits has no HEAD to resolve, which is not an error +/// here: the sha comes back empty — the digest is then the patch alone, exactly +/// as `digest` has always handled it — and the name falls back to `HEAD`, which +/// is what `currentBranch` answers for the same case. +pub const Head = struct { + sha: []const u8, + branch: []const u8, +}; + +pub fn head(repo: Repo, gpa: std.mem.Allocator, io: std.Io) Head { + const out = runFor(gpa, io, repo.path, &.{ + "rev-parse", "HEAD", "--abbrev-ref", "HEAD", + }, poll_timeout_s) catch return .{ .sha = "", .branch = "HEAD" }; + + // Two lines, in the order the arguments were given: the sha, then the name. + var it = std.mem.splitScalar(u8, trim(out), '\n'); + const sha = trim(it.next() orelse ""); + const branch = trim(it.next() orelse ""); + return .{ + .sha = sha, + .branch = if (branch.len == 0) "HEAD" else branch, + }; +} + pub fn currentBranch(repo: Repo, gpa: std.mem.Allocator, io: std.Io) []const u8 { const out = run(gpa, io, repo.path, &.{ "rev-parse", "--abbrev-ref", "HEAD" }) catch return "HEAD"; @@ -360,13 +399,14 @@ pub fn revision( io: std.Io, ctx: model.DiffContext, opts: Options, + head_sha: []const u8, ) Error![]const u8 { try validateCommit(ctx.commit); - const patch = try run(gpa, io, repo.path, try diffArgs(gpa, ctx, opts, &.{ + const patch = try runFor(gpa, io, repo.path, try diffArgs(gpa, ctx, opts, &.{ "--no-color", "--find-renames", - })); - return digest(repo, gpa, io, patch); + }), poll_timeout_s); + return digestOf(gpa, head_sha, patch); } /// The digest itself, given a patch already in hand: the patch, hashed, plus @@ -387,9 +427,15 @@ fn digest( ) Error![]const u8 { // Best-effort: a repository with no commits has no HEAD to resolve, and the // patch on its own is still a usable digest. - const head = run(gpa, io, repo.path, &.{ "rev-parse", "HEAD" }) catch ""; + const at = run(gpa, io, repo.path, &.{ "rev-parse", "HEAD" }) catch ""; + return digestOf(gpa, trim(at), patch); +} + +/// The hash itself, for a caller that has already resolved HEAD — which the +/// poll has, because it needs the branch name from the same `rev-parse`. +fn digestOf(gpa: std.mem.Allocator, head_sha: []const u8, patch: []const u8) Error![]const u8 { var hasher = std.hash.Wyhash.init(0); - hasher.update(trim(head)); + hasher.update(head_sha); hasher.update(patch); return std.fmt.allocPrint(gpa, "{x}", .{hasher.final()}); } @@ -660,6 +706,16 @@ fn run( io: std.Io, dir: []const u8, args: []const []const u8, +) Error![]const u8 { + return runFor(gpa, io, dir, args, timeout_s); +} + +fn runFor( + gpa: std.mem.Allocator, + io: std.Io, + dir: []const u8, + args: []const []const u8, + seconds: u32, ) Error![]const u8 { var argv: std.ArrayListUnmanaged([]const u8) = .empty; try argv.ensureTotalCapacity(gpa, args.len + 1); @@ -671,7 +727,7 @@ fn run( .cwd = .{ .path = dir }, .stdout_limit = .limited(max_output), .stderr_limit = .limited(64 * 1024), - .timeout = .{ .duration = .{ .raw = .fromSeconds(timeout_s), .clock = .awake } }, + .timeout = .{ .duration = .{ .raw = .fromSeconds(seconds), .clock = .awake } }, }) catch |err| { std.log.warn("review: git {s}: {s}", .{ args[0], @errorName(err) }); return error.GitFailed; @@ -850,8 +906,8 @@ test "diffArgs picks the right git subcommand for each selection" { }), one); // An empty base is HEAD, which is what a review opens on. - const head = try diffArgs(a, .{ .uncommitted = true }, .{}, &.{}); - try std.testing.expectEqualDeep(@as([]const []const u8, &.{ "diff", "HEAD" }), head); + const bare = try diffArgs(a, .{ .uncommitted = true }, .{}, &.{}); + try std.testing.expectEqualDeep(@as([]const []const u8, &.{ "diff", "HEAD" }), bare); } test "oversized counts lines, and files for the binary case" { diff --git a/web/src/App.tsx b/web/src/App.tsx index 87877c3..1ba16f2 100644 --- a/web/src/App.tsx +++ b/web/src/App.tsx @@ -1,7 +1,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { parseDiff, type ViewType } from 'react-diff-view'; -import { api, tabId } from './api'; +import { ApiError, api, tabId } from './api'; import type { Comment, DiffContext, DiffPayload, DraftTarget, RepoState } from './types'; import { buildAnchors, isOutdated } from './lib/anchor'; import { pathOf } from './lib/filetree'; @@ -58,6 +58,23 @@ const MIN_DIFF = 420; // entirely while the pane is hidden or already known to be stale. const POLL_MS = 8000; +// ...but "isn't free" stops being true when a review is open in seven tabs and +// the repository is a big one being built in the terminal next door. Every pane +// polling on the same fixed interval is how the app ends up with a git process +// running most of the time, and the request an agent is waiting on ends up +// queued behind polls whose answers nobody reads. +// +// So the interval is a floor, not a period: each pane spaces its next poll by +// what the last one actually cost, which means panes on a slow repository — the +// only ones expensive enough to matter — quietly ask less often, and a pane on a +// small one carries on at POLL_MS. `POLL_COST` is how many times the last poll's +// duration to wait, and the server's own "git is busy" refusal (503) counts as +// expensive whatever it cost, because it is the app telling this pane it is one +// of too many. +const POLL_MAX_MS = 60000; +const POLL_COST = 6; +const POLL_BUSY_MS = 20000; + const CTX_KEY = 'review-ctx-by-repo'; const IGNORE_WS_KEY = 'review-ignore-whitespace'; @@ -411,17 +428,29 @@ export default function App() { let canceled = false; let busy = false; + let timer: number | undefined; + + // Each poll arms the next one rather than a fixed interval doing it, so the + // spacing can answer to what the last one cost — see POLL_MAX_MS. + const arm = (ms: number) => { + window.clearTimeout(timer); + if (!canceled) timer = window.setTimeout(check, ms); + }; const check = async () => { // A hidden pane is a pane nobody is reading. It gets checked the moment it // comes back instead, which is when the answer matters. - if (canceled || busy || document.hidden) return; + if (canceled || busy) return; + if (document.hidden) return arm(POLL_MS); busy = true; const seq = reqRef.current; + const started = performance.now(); + let next = POLL_MS; try { const { revision: now, branch: on } = await api.revision(ctx, { ignoreWhitespace: ignoreWs, }); + next = Math.max(POLL_MS, Math.round((performance.now() - started) * POLL_COST)); // A load that started while this was in flight has already answered the // question, with a revision this closure doesn't know about. if (canceled || seq !== reqRef.current) return; @@ -432,14 +461,21 @@ export default function App() { // you were reading, and the banner is what offers to move that. if (on && on !== branch) loadRepo(); if (now && now !== revision) setStale(now); - } catch { - // A failed poll says nothing about the diff — the next one will. + } catch (e) { + // A failed poll says nothing about the diff — the next one will. A 503 + // says something about the app, though: too many panes are asking git + // for too much at once, and this one is part of that. + next = + e instanceof ApiError && e.status === 503 + ? POLL_BUSY_MS + : POLL_MS; } finally { busy = false; + arm(Math.min(POLL_MAX_MS, next)); } }; - const timer = window.setInterval(check, POLL_MS); + arm(POLL_MS); const onVisible = () => { if (!document.hidden) check(); }; @@ -447,7 +483,7 @@ export default function App() { window.addEventListener('focus', onVisible); return () => { canceled = true; - window.clearInterval(timer); + window.clearTimeout(timer); document.removeEventListener('visibilitychange', onVisible); window.removeEventListener('focus', onVisible); }; diff --git a/web/src/api.ts b/web/src/api.ts index 975aa5c..03ca099 100644 --- a/web/src/api.ts +++ b/web/src/api.ts @@ -32,6 +32,20 @@ export const tabId = (() => { return match ? decodeURIComponent(match[1]) : ''; })(); +// An error the server answered with, carrying the status alongside the sentence. +// The status matters in one place: a 503 from the poll is the server rationing +// git rather than anything being wrong, and the poll backs off instead of asking +// again on the same tick — see the poll effect in App. +export class ApiError extends Error { + status: number; + + constructor(status: number, message: string) { + super(message); + this.name = 'ApiError'; + this.status = status; + } +} + async function json(res: Response): Promise { if (!res.ok) { const body = await res.text(); @@ -39,11 +53,11 @@ async function json(res: Response): Promise { // beats surfacing a status code, because it is written for a person. try { const parsed = JSON.parse(body) as { error?: string }; - if (parsed?.error) throw new Error(parsed.error); + if (parsed?.error) throw new ApiError(res.status, parsed.error); } catch (e) { if (e instanceof Error && e.message && !e.message.startsWith('Unexpected')) throw e; } - throw new Error(`${res.status} ${res.statusText}: ${body}`); + throw new ApiError(res.status, `${res.status} ${res.statusText}: ${body}`); } if (res.status === 204) return undefined as T; return res.json() as Promise;