Skip to content

feat: workspace pool management (listing, reclaim, expand, shrink) - #58

Open
akozak-gd wants to merge 6 commits into
mainfrom
feat/workspaces-management
Open

feat: workspace pool management (listing, reclaim, expand, shrink)#58
akozak-gd wants to merge 6 commits into
mainfrom
feat/workspaces-management

Conversation

@akozak-gd

@akozak-gd akozak-gd commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Adds operator management of the workspace pool: a TUI screen to see what exists, plus the API behind it to grow the pool and get workspaces back.

Entrypoint

Press w on the sessions screen. Or read docs/backend/workspace-pool-management.md.

4 set(s), 12 workspace(s) — 1 ready to allocate   allocated 4  available 5  cleaning 2  stuck 1

Set 01          READY
  ws-01-1   ○ AVAILABLE   akozak-gd/specflow-workspace1   last run est-bfea24f71300
Set 03          BLOCKED  ws-03-1 is cleaning; ws-03-3 is stuck
  ws-03-1   ◐ CLEANING    akozak-gd/specflow-workspace7   cleaning, 1h 30min of grace left
  ws-03-3   ✗ STUCK       akozak-gd/specflow-workspace9   archive push failed
Set 04          BLOCKED  ws-04-1 is allocated; ws-04-2 is available but not clean-verified
  ws-04-1   ● ALLOCATED   akozak-gd/specflow-workspace10  est-2ff (failed) — retry would be lost
  ws-04-2   ○ AVAILABLE   akozak-gd/specflow-workspace11  not clean-verified — cannot be allocated
Key Action
o open the workspace's GitHub repo
c reclaim the workspace, or the whole set from its header
x add sets — progress streams into a log pane
d remove a set from the pool

Why

Three gaps made this necessary rather than cosmetic:

  • /pool/status returns counters only — no workspace ids, no repo_url, no set membership. Nothing could render a listing.
  • No API path for expansion. GitHub repo creation lived only inside scripts/create_generation_session_repos.py, so growing the pool meant running a host-side script with the right SQLITE_DB_PATH.
  • Set-level cleaning was faked client-side — the TUI rebuilt the three member ids from the ws-NN-N convention and fired three requests, swallowing partial failures. STUCK recovery existed in the service with no HTTP route at all.

Endpoints

All under /api/v1/workspace, all admin-gated.

Method Path
GET /pool/sets per-set listing: members, repo, lock owner, verdicts
POST /pool/reclaim return workspaces or whole sets to AVAILABLE
POST /pool/expand add N sets — 202 + job id, polled
GET /pool/expand/{job_id} expansion progress
POST /pool/shrink remove slots, keeping the GitHub repos

Details worth a reviewer's attention

Reclaim dispatches on state, from one rule. classify_reclaim decides what reclaiming a workspace would do — finish an interrupted cleaning, force-clean a stale AVAILABLE one, recover a STUCK one, or release an ALLOCATED one whose generation has finished. Both the listing badge and the dispatch read it, so what the UI offers cannot drift from what runs. An ALLOCATED workspace whose generation is still live is refused, naming the generation.

Nothing is destroyed: cleanup archives the tree to branch {generation_id} and verifies the push with git ls-remote before wiping. Only in-place retry is lost, flagged as retry_lost_on_reclaim so the confirm dialog can warn. confirmed_by comes from the authenticated caller, not the request body — a forged value in the body is ignored.

Expansion has to be a background job. P10Y has no create-repository API; a repo becomes usable only after Compass re-fetches the connection (~60s) and metrics are enabled (up to 5 min).

Two invariants:

  • Rows are seeded last, so a slot is never published before its repo has a P10Y id.
  • replace=False. The bootstrap script seeds with replace=True, which resets documents to available/locked_by=None — running that as "expansion" would drop an in-flight allocation.

New repos continue the pool's own naming, inferred from existing repo_urls rather than a configured default: the script default (generation-workspace) and WORKSPACE_REPO_PREFIX (specflow-workspace) disagree, so a default would fork the naming into two families. A pool of 3 sets (repos 1–9) expanded by 3 gives ws-04-*ws-06-* on repos 10–18, nothing renumbered.

Job state is in-memory. Every step is idempotent, so a restart costs a re-run rather than correctness; the progress endpoint 404s and says so.

Shrink keeps the repos. Each workspace repo holds the archive/{generation_id} branches of every run that used it — the only remote copy of that code. Deleting repos would destroy generation history, so shrink removes pool rows only. Only AVAILABLE + clean-verified workspaces are removed; anything else is refused with "reclaim it first".

Consolidations. Extracted GitHubAPIClient and the P10Y discovery dance into app/services (the script now delegates and shrank ~530 lines, its 66 tests unchanged). Promoted two duplicated values to shared constants: the terminal generation statuses and WORKSPACES_PER_SET. Also removed the dashboard's clear ws flow, which the new screen supersedes — that took tui/actions.py and four render helpers with it (−312 lines).

Admin-role fix (df52cf8), separable. require_admin's docstring promised "*" grants admin while its code required literal "admin" — and the sibling _has_admin_permission did accept "*". Key creation rejects "*" outright (test_wildcard_permission_rejected), so this narrows to one predicate and corrects the docstring plus a curl example that taught the rejected value.

Verification

  • Backend 2234 passed, 36 skipped; mcp_server 787 passed. The 3 TestStartupGate failures are pre-existing and environment-dependent (the gate picks StartBackendProcessScreen under BACKEND_RUNTIME=process) — same 3 on main.
  • ruff clean. mypy at its 4 pre-existing errors, confirmed by stashing.
  • Average cyclomatic complexity 3.61 → 3.60.

Not covered by tests: the P10Y re-sync poll against real Compass credentials. It is the step most likely to break and no unit test can prove it — worth one real x against a GitHub org before trusting expansion.

Known, from self-review — worth resolving in review

Raising these here rather than leaving them for a reviewer to find:

  1. /pool/reclaim and /pool/shrink are not pool-scoped. /pool/sets filters by the caller's workspace_pool, but the two destructive routes act on any workspace_ids given, and reclaim_workspace / shrink_pool never read workspace_pool. Ids are predictable (ws-NN-N), so a pool-scoped admin key can reclaim or de-register another pool's workspace. verify_generation_session_owner already 403s on a pool mismatch, so the boundary exists — these routes just don't honour it. Also pool = getattr(request.state, "workspace_pool", None) leaves None = "all pools" on the reclaim path, where /pool/expand defaults to DEFAULT_WORKSPACE_POOL. The pre-existing /force-deallocate, /{id}/clear, /{id}/force-release share the gap.
  2. assert in workspace_pool_expansion._require_p10y_config (mypy narrowing only). Forbidden outside tests — under -O it is stripped and the clear WorkspacePoolExpansionError becomes a TypeError.
  3. Expansion reports done on a P10Y metrics timeout. poll_repository_status returns last-known statuses rather than raising, by design ("the caller decides"), and expand_pool discards the return value — so the job logs Timed out after 5 min… and then pool is ready, with phase=done, error=None.

Lower priority: shrink_pool's docstring claims re-expansion re-adopts the removed repos, which only holds when the highest-numbered set was removed (numbering is derived from the survivors); expansion derives repo/set numbering per-pool although both namespaces are global, and a resulting collision is swallowed by already_existed / replace=False skips; two new ci/check_state_writes.sh false positives need the # state-ok marker (that guard is already red on main and is wired into neither the Makefile nor CI).

Follow-ups

Display-only, no new endpoints: show the archive/{generation_id} branch for a completed run in the detail pane (a completed set currently reads only last run <id>, which under-reports that the code is still cloneable), and surface the scheduled_for_wipe_at warning — it is returned by the API but not rendered.

GitHub repository creation and the P10Y/Compass discovery dance lived
only inside scripts/create_generation_session_repos.py, so nothing under
app/ could provision a workspace repo. Move both into app/services and
have the script delegate, leaving one implementation of the details that
matter: owner-type detection (orgs and personal accounts use different
creation endpoints), auto_init (the pool clones every repo_url, and an
empty repo has no branch to clone), and the connection re-fetch that
makes new repos visible to Compass.

Progress is reported through an on_progress callback instead of print, so
a caller that is not a terminal can consume it.

The script keeps its numeric-range interface and dict-shaped returns via
thin adapters; its 66 tests pass unchanged.
Operators had no way to see or manage the pool. /pool/status returns
counters only — no workspace ids, no repo_url, no set membership — so
nothing could render a listing, and growing the pool meant running a
host-side script with the right SQLITE_DB_PATH.

Adds four endpoints, all admin-gated:

  GET  /pool/sets      per-set listing with repo, lock owner, verdicts
  POST /pool/reclaim   return workspaces or whole sets to AVAILABLE
  POST /pool/expand    add N sets (202 + job id, polled)
  POST /pool/shrink    remove slots, keeping the GitHub repos

Reclaim dispatches on state — finish an interrupted cleaning, force-clean
a stale AVAILABLE one, recover a STUCK one (previously reachable only from
Python), or release an ALLOCATED one whose generation has finished.
classify_reclaim makes that decision once and both the listing badge and
the dispatch read it, so what the UI offers cannot drift from what runs.
An ALLOCATED workspace whose generation is still live is refused. Nothing
is destroyed: cleanup archives the tree to branch {generation_id} and
verifies the push before wiping, so only in-place retry is lost — flagged
as retry_lost_on_reclaim. confirmed_by comes from the authenticated
caller, not the request body.

Expansion is a background job because P10Y has no create-repository API:
a repo becomes usable only after Compass re-fetches the connection (~60s)
and metrics are enabled (up to 5 min). New repos continue the pool's own
naming, inferred from existing repo_urls rather than a configured default
— the script default (generation-workspace) and WORKSPACE_REPO_PREFIX
(specflow-workspace) disagree, so a default would fork the naming. Rows
are seeded last and with replace=False, so a slot is never published
before its repo has a P10Y id and a live document is never reset to
available. Job state is in-memory; every step is idempotent, so a restart
costs a re-run rather than correctness.

Shrink removes pool rows only. Each repo holds the archive/{generation_id}
branches of every run that used it, so deleting repos would destroy
generation history.

Also promotes two duplicated values to shared constants: the terminal
generation statuses (state machine, wipe job, reclaim) and
WORKSPACES_PER_SET (allocation, seeding, expansion).
require_admin's docstring promised that "*" grants admin, but its code
accepted only literal "admin" — while the sibling _has_admin_permission,
used for the ownership bypass, did accept "*". Two rules for one concept.

Key creation rejects "*" outright (test_wildcard_permission_rejected), so
honouring it would grant admin through a value the API refuses to issue.
Narrow _has_admin_permission to match, have require_admin delegate to it
so there is a single predicate, and correct the docstring and the curl
example that taught the rejected value.
Pool state was invisible. The only pool-aware code was the dashboard's
clear flow, which could free one already-CLEANING set belonging to the
run you happened to be looking at.

Adds WorkspacesScreen, reached with w from the sessions overview (pool
management spans every generation, so it belongs there rather than on the
per-generation dashboard). Lists every set with its members, each showing
a status pill, org/repo, and the context an operator needs: who holds it,
the CLEANING grace countdown, a STUCK reason, or why an available
workspace cannot be allocated. Blocked sets name the offending member.

  o  open the workspace's GitHub repo
  c  reclaim the workspace, or the whole set from its header
  x  add sets, streaming backend job progress into a log pane
  d  remove a set from the pool

Destructive actions confirm with a countdown and say what actually
happens — that work is archived and pushed before a wipe, and that
removing a set keeps its GitHub repositories. Reclaim and shrink read the
eligibility verdict computed by the backend rather than re-deriving it, so
the UI cannot offer an action the server would refuse.

Replaces the client-side set clear, which rebuilt the three member ids
from the ws-NN-N convention and fired three separate requests, swallowing
partial failures; membership now comes from the database in one call. Its
tests are rewritten to the new contract rather than adjusted to pass.

Row and message formatting is pure functions in render.py, unit-tested
without a running app.
Adds docs/backend/workspace-pool-management.md covering the endpoints, the
per-state reclaim dispatch, why expansion must be a background job, and
why shrink never deletes GitHub repositories.

Corrects references that no longer match the code:

- CLAUDE.md named Firestore as the persistence decision; the runtime is
  SQLite by default (DATABASE_TYPE=sqlite in docker-compose and the
  quickstart env), with Firestore one of four backends behind IDatabase.
- db_adapter.py labelled its constants "Firestore collection names" and
  promised a native async Firestore client in a later phase.
- The quickstart env claimed repos are named {prefix}-ws-1; the code
  produces {prefix}1.
Two dashboard bindings that no longer earn their place:

clear ws (w) is unused and superseded. Reclaiming is a pool operation
spanning every generation, so it belongs on the workspaces screen where
the whole pool is visible — the dashboard version could only free a set
that was already CLEANING and happened to belong to the run on screen.

open ws had both o and a hidden enter bound to it. Opening a workspace is
what selecting it does, so enter becomes the visible binding and o goes.

Removing the clear flow makes its whole support chain dead: tui/actions.py
existed only to wrap cmd_clear_workspace, and render lost run_set_number,
set_number_from_workspace_id, clear_ws_eligible and
clear_ws_ineligible_message. Net -312 lines.

The CLI keeps `specflow clear-workspace --set N`; the capacity message
printed when no workspaces are free still points at it.
@akozak-gd
akozak-gd marked this pull request as ready for review August 3, 2026 08:43

@awrobel-gd awrobel-gd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments:

  • good that you expanded the Git library and replaced the boot script with just calls to that library
  • I see workspace pools give us some trouble now - strictly a feature for managed deployment. In local i think we will never ever need it. We could remove it if it simplifies the PR (maybe as another PR)
  • This PR is 6k lines - for sake of sanity we could merge separately the libraries code and follow up would rewire. Finally last PR would be the enabler in TUI (up to you)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants