Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Three unresolved review threads, all real defects, plus a stray debug
print and a grammar keyword rename.
visit_number read node.text, but the `number` rule is `<regex> ws*` and
`ws` matches `/* ... */` as well as whitespace, so a comment directly
after a numeric literal made float() raise on otherwise valid Fusion
statements. Read the regex child's text instead: unlike
flatten(visited_children)[0] it is not confused by the optional fraction
group, which matches empty for a bare integer.
UPLOAD CUSTOM MODEL built the remote path from the whole local_path, so
an absolute or nested path replayed the local directory tree into the
models space (model_name/tmp/weights.bin). Use the basename. The
directory branch was already correct -- upload_folder re-bases each
entry against local_root.
Both upload_folder implementations normalized their remote prefix
without strip_leading, unlike the listdir/download_folder call sites
that already passed it. FileSpace._upload builds
`files/fs/{location}/{path}`, so a leading '/' produced a doubled
slash; the `path = local_path` fallback could leak a './' prefix too.
Also drops the stray print(visited_children) from visit_compound, and
renames the CREATE CLUSTER scale-factor clause from WITH SCALE FACTOR to
USING SCALE FACTOR, with the rule name following the keyword as the rest
of that grammar does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pyproject.toml's addopts sets -n 3, so every workflow step inherited xdist. The HTTP/Data API steps must not: they set SINGLESTOREDB_INIT_DB_URL, so load_sql's setup connection is MySQL and takes the `SET GLOBAL HTTP_PROXY_PORT` + `RESTART PROXY` branch (singlestoredb/tests/utils.py:227) once per worker, and a proxy restart drops whatever HTTP request another worker has in flight. Adds -n 0 to the HTTP steps in code-check.yml and coverage.yml. The https smoke-test step gets it too: it avoids the restart -- with no INIT_DB_URL its setup connection is itself HTTP, so that branch is skipped -- but it drives the same Data API, so it should not be the lone parallel run of it. The MySQL steps are unaffected: http_port stays 0 for a non-http URL, so the restart never happens and they keep the parallel default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A cl-test-shared-0-* cluster was found ACTIVE and untracked after a run. Four separate holes let that happen; the first is silent, which is why it went unnoticed. 1. A creation call that fails while waiting left nothing tracked at all. Every creator brings the deployment into existence and only then waits for it -- create_cluster does its get_cluster before _wait_on_state (v2/cluster.py:1426) -- so a timeout, a transient error, or a Ctrl-C raises after the server already has a live, billable deployment. Since tracking wrapped only the return value, nothing registered it: no per-class sweep, no end-of-session sweep, and no summary line. The shared cluster pool is the worst-exposed caller, being the only one that waits with wait_on_active=True and wait_timeout=1200. _CREATORS now carries a finder per creator, and the wrapper looks the orphan up by name and tracks it when the call raises. BaseException, not Exception, so an interrupt mid-wait reaps too. 2. cleanup_tracked dropped every entry before trying to terminate it, and only logged a failure -- so one transient error leaked the deployment permanently, with no retry and no mention in the summary. Entries now stay tracked until they are confirmed gone or actually terminated. 3. _is_gone treated any refresh failure as "already gone", which is right for a 404 and wrong for a 503: it skipped the termination and left the cluster running. Only a 404 counts as gone now; anything else reports still-live, since a redundant terminate costs one round trip and a missed one costs money. 4. Both the sweep and the container cleanup lived only in pytest_unconfigure, which a cancelled CI job or a killed xdist worker never reaches. Adds atexit and SIGTERM fallbacks -- verified to fire on both an unhandled exception and a signal, preserving exit code 143. SIGKILL stays unreachable; cleanup_deployments.py is the net for that. conftest also now reports anything still tracked after the final sweep, naming cleanup_deployments.py, so a leak is loud instead of silent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit aae8466. Configure here.
Bugbot found both in the leak-prevention machinery added by aae8466, and each one can leave a real, billable cluster running. `_tracking_wrapper` guarded orphan recovery with `_is_mocked(receiver)`, but that helper looks for a `_manager` attribute and the receiver here is the manager, which has none -- so a unit test driving a real manager with a patched `_post` read as live and the recovery fired an actual management API GET. `_creator_is_mocked` is the helper that inspects the receiver's own transport, and it already handles both receiver shapes. The out-of-band sweeps (SIGTERM, atexit, unconfigure) only walked `_tracked`, which a create that has POSTed and is blocked in `wait_on_active` has not entered yet: the wrapper tracks on return and recovers in its `except`, and a killed process runs neither. So creations are now listed in `_in_flight` for their duration, and a whole-session sweep drains that list through the same `_recover_orphan` first. Whoever claims an entry -- the sweep or the wrapper's `except` -- is the one that recovers it, so nothing gets tracked twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The UDF stage:// handling and the Fusion cluster handlers went through
_manage_workspaces_v1() and spoke workspace-group vocabulary, so they
only worked against v1 even when the caller's deployment was a cluster.
- functions/ext/{asgi,mmap}.py: resolve stage through the neutral
get_stage(hostname) instead of a v1 workspace manager, and translate
its RuntimeError into a ValueError that names what is missing.
- fusion/handlers/utils.py: add _deployment_param() plus the shared
_DEPLOYMENT_KEYS/_GROUP_SPELLING_HINT, so a handler can accept either
spelling and hint at the other one when lookup fails.
- fusion/handlers/cluster.py: resolve regions off the manager's
TTL-cached regions list, matching case-insensitively.
- management/v1/, management/v2/: docstrings document the classes and
functions rather than narrating the v1-to-v2 migration.
- tests/test_fusion.py: cover the deployment-key handling and hints.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The default version was spelled 'v2' as a literal in the config option registration, in Manager.default_version, in FilesManager, and in DEFAULT_CLUSTER_VERSION, so retargeting the SDK at a new version meant finding all of them and hoping the option and the classes stayed in step. Add singlestoredb/_management_version.py holding DEFAULT_MANAGEMENT_VERSION and DEPRECATED_MANAGEMENT_VERSION. It imports nothing, which is what lets both config.py and management/ read it: config is imported before management, and management.manager imports config, so neither package can host the constants. Every literal that meant "the current default" now comes from there -- the option default, _version_import.DEFAULT_VERSION, Manager, and DEFAULT_CLUSTER_VERSION. FilesManager's override is deleted outright; it only ever restated Manager's value. Literals stay where the point is a specific version rather than the current one: the default_version of a version-specific class, and the v1 guards in manage_workspaces() and manage_clusters(). default_version deliberately still does not read the option. Nor get_default(): Option.__init__ folds the environment variable into the registered default, so a class reading it would take a v1 URL from SINGLESTOREDB_MANAGEMENT_VERSION=v1. The new subprocess test covers that hole, which no in-process set_option() can reach. Also: docstrings across the management modules document the code rather than narrating the v1-to-v2 migration or carrying dated "verified live" claims, _manage_workspaces_v1's error no longer advises setting an option it never reads, and ADR 0001 plus the two plan documents record the constant and drop claims that are no longer true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It changes default management API behavior (v2-by-default), restructures many public entry points, and updates CI/test execution semantics, making the regression surface too broad for automated approval.
Review details
Suppressed comments (1)
singlestoredb/notebook/_objects.py:208
- Same late-bound
attrissue as inStage: this annotations loop usesattrfrom the priordir(_OrganizationBase)loop when callingfunctools.update_wrapper, so wrapper metadata comes from an unrelated member and depends on loop ordering. Consider returningproperty(wrap)here (or wrapping against a resolved member form) to keep behavior stable and intent clear.
- Files reviewed: 73/78 changed files
- Comments generated: 1
- Review effort level: Lite
The four notebook proxy classes build their properties in two passes: one over dir() of the class they front, one over its __annotations__. The second pass ran functools.update_wrapper(wrap, attr) against `attr`, which only the *first* pass ever assigns -- so every annotation-backed property took its __name__, __doc__ and __wrapped__ from whichever public dir() member happened to be last. There is no attribute object behind an annotation to copy metadata from, so there was never a right value for that call to find; had a fronted class carried annotations but no public dir() members, `attr` would have been unbound and __new__ would have raised NameError. Return property(wrap) instead, and drop the is_method parameter these four factories accepted and never read. Renaming them to make_annotation_wrapper keeps mypy from reading the pair in each __new__ as one conditionally redefined function, and says which pass is which. The dir() loops keep update_wrapper: there `attr` is the member being proxied, and copying its signature and docstring is the point. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A deployment is named the same way whatever kind it is, so a qualified IN CLUSTER resolved exactly where the bare IN already did -- get_deployment() treats every spelling as one code path. The clause was six copies of a four-line grammar block buying nothing, plus a first-match ordering hazard: let in_cluster fall below in_deployment and IN CLUSTER 'x' silently parses as a deployment named CLUSTER. The six handlers now take a bare IN, with IN GROUP kept only because it already parses. IN CLUSTER never reached main, so there is nothing to deprecate. The v1 WORKSPACE grammar is untouched: it is on its way out, and giving it a new spelling would change behaviour there for no gain. Also retarget the two messages that pointed users at IN CLUSTER -- the IN GROUP synonym hint and the SINGLESTOREDB_WORKSPACE_GROUP error. The grammar test inverts to asserting the spelling is absent and that SHOW STAGE FILES IN CLUSTER 'c1' fails to parse. That last assertion is the point: the risk in removing an alternation branch is the spelling reinterpreting as a deployment named CLUSTER rather than erroring. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It was the one command in handlers/workspace.py left without a _deprecated_by pointer, on the grounds that SHOW CLUSTER REGIONS drops the ID column and so is not a drop-in. But the reason to warn is the route, not the columns: this command reads the v1 API, and that is what is going away. A caller holding a v1 region ID needs to hear it now rather than when the route stops answering. Point it at SHOW CLUSTER REGIONS and record the column difference where a reader will meet it -- a Remark on SHOW REGIONS itself, next to the ID column it is about. The test asserted the exemption, so it asserted the opposite of what we now want; it now requires that nothing in the module is undeprecated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
WITH PROVIDER reads as though it sets something on the cluster, which it does not: it narrows which region IN REGION means when two providers offer a region under the same name. USING says that -- and matches USING SCALE FACTOR, the other clause in this grammar that qualifies a value rather than setting one. Renames the rule as well as the keywords, so the params key follows, in both handlers that take the clause: CREATE CLUSTER, where it is optional, and CREATE STARTER CLUSTER, where both halves of the region are required. The old spelling is not accepted -- this grammar has not shipped, so there is nothing to keep working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
That variable names a project of the inference API, not of the cluster
management API. The two are separate namespaces and the notebook
environment reports different IDs for them: a notebook attached to a
cluster in Standard Project publishes an ID that
GET /v2/projects/{id} answers 404 project not found for. Reading it in
_resolve_project_id() therefore broke CREATE CLUSTER in every notebook,
in every organization -- the failure was a hard KeyError, since an
environment-derived ID was treated as authoritative.
Priority two becomes the project of the deployment the code is running
in, read off SINGLESTOREDB_WORKSPACE via GET /v2/clusters/{id}. That is
a better default than the variable ever was: a new cluster lands beside
the one it was created from, which makes IN PROJECT optional in a
notebook even in an organization with several projects. A deployment
that cannot be read -- outside a notebook, a starter cluster, a stale ID
-- falls through silently, because there are further defaults to try.
get_project_id() stays for its one legitimate caller, inference_api.py,
and is now where the distinction is documented. The Fusion get_project()
resolves the IN PROJECT clause and nothing else. The test suites used the
same variable to pick a deployment target, which was never its meaning
either; that override is now SINGLESTOREDB_TEST_PROJECT.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Uploading one file made two identical exists() requests -- upload_file checked, then _upload checked again -- and ended with an info() request that all three Fusion upload handlers threw away. The check now lives only in _upload, and _upload takes a private fetch_info flag so a caller that discards the FilesObject does not pay for building one. Both upload_file methods delegate to a shared _upload_local_file on FileLocation, which is also what the Fusion handlers call; it wraps the local open() in a with block, since the conflict is now raised after the handle exists. A Stage or file space upload drops from four requests to three, from six to five with overwrite, and a folder upload saves one per file. The public upload_file still returns a populated FilesObject. CountingManager in tests/utils.py is the harness for this: it stands in for a Manager, simulates the filesystem, and records every request, so the round-trip count of an operation is assertable without a deployment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_create_drop_workspace_group named its subject
'Create WG Test {id(self)}', which matches nothing in
cleanup_deployments.PATTERNS. So every run that died between the create
and the drop -- or whose terminate failed, which that test swallowed
silently -- left a live workspace group that the maintenance sweep
reported as "No leftover test deployments found". They accumulate until
someone reaps them by hand through the management API.
The name is now a random hex token, which id(self) never was: an address
repeats across processes, so two workers could pick the same one. The
pattern added for it accepts hex, and decimal is a subset, so the groups
older runs stranded are reaped by the same sweep.
The general fix is --show-unmatched: find_leftovers now also returns the
live deployments whose names it does not recognize, so the next test that
invents a name is visible instead of silently filling the organization.
The two parse-failure cluster names get an id suffix for the same reason,
and the test's own cleanup reports a failed terminate rather than
discarding it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--show-unmatched turned up seven stranded workspace groups named 'Stage Fusion Testing <n> <id>' and 'Files Fusion Testing <id>', 64 to 232 hours old. Those names were retired on this branch -- TestStageFusion moved to v2 clusters and then to the shared pool, and TestFilesFusion stopped creating a group it never read -- but they are still live in the organization and still billing. They also keep arriving, and this is the reason strays recur: none of the cleanup machinery is on main. No utils.track(), no per-class sweep, no cleanup_deployments.py -- a run there has only tearDownClass, so a killed run or a setUpClass that raises leaks a group permanently, under the old names. LEGACY_PATTERNS matches them until main carries the sweep. Not matched, on purpose: groups named 'Group <hex8>'. No revision of this repo generates that, so a pattern for it would be a guess with a live workspace group on the other end. --show-unmatched will keep reporting it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No revision of this repo generates these names, so attribution cannot justify the pattern; it is here because the deployments are in the organization and are being billed. The eight-character floor is the guard: 'Group 1' and 'Group 2' are what a person or the portal produces, and a bare [0-9a-f]+ would reap them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
80854d8 renamed the clause but only caught the sites spelled in upper case; this one builds the statement in lower case from an f-string, so it still said 'with provider' and the grammar rejected it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Stage 2 of docs/stage-upload-round-trips-plan.md: Cluster.project and StarterCluster.project resolve their reported projectID on first read rather than in from_dict, so resolving a deployment by name no longer drags GET /v2/projects behind the cluster listing it needs. The one-hour ttl_property on ClusterManager.projects still matters -- SHOW CLUSTERS EXTENDED reads .project per row and one fetch serves them all. Stage 1c: Stage._upload and FileSpace._upload fetched the same metadata twice, once through exists() and again through remove()'s is_dir(). They fetch it once now, through the shared FileLocation._info_or_none, and branch on the object; remove() keeps its own is_dir() for its other callers. An UPLOAD FILE TO STAGE ... IN '<name>' costs three requests instead of four, or four instead of six with OVERWRITE. The request-count harness grew a CountingClusterManager and run_fusion_statement so the count of a whole statement is pinned, not just the count of a Stage call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Cluster.region was the one eager resolve Stage 2 left behind: a payload carrying a region made from_dict match it against ClusterManager.regions, so a realistic cluster listing paid a GET /v2/regions that an upload never looked at. It now stores the reported name and resolves on first read, falling back to a Region built from what the cluster itself reported -- the same shape as the lazy project. Neither lazy value is in vars(cluster), so vars_to_str would drop both from str(cluster), and resolving them in __repr__ would make printing a cluster issue two requests. vars_to_str takes an extra= mapping for this; Cluster.__str__ passes the resolved object when something has already read the property and the reported ID / name otherwise, so printing stays free. SHOW CLUSTERS EXTENDED and SHOW STARTER CLUSTERS EXTENDED report ProjectName rather than ProjectID, which is what the plan said all along. _project_from_id's '<unknown>' fallback means the name is always populated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The creation wrapper worked out from its receiver that a call was going through a mocked manager, used that to skip orphan recovery, and then handed the result to track() anyway. track() judges only what it is given, and is deliberately biased toward "real" for anything it cannot place -- a cluster left running bills money, a redundant terminate costs one round trip -- so a stubbed get_cluster returning a Cluster with _manager=None, or a bare 'sentinel' string, registered as a live deployment. The unit tests then ended with nine phantom deployments in _tracked, a failed refresh and a failed terminate logged for each, and a "9 deployment(s) left live" banner. Nothing had leaked. The cost is that the banner is the only thing that reports a genuinely leaked cluster, and constant phantom noise is how it stops being read. The receiver's verdict now decides whether the return value is tracked too. It is the authoritative one: nothing a mocked creator returns names a deployment that exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
IN GROUP was wired as a synonym for a bare IN, so it carried the
deployment_id/deployment_name placeholders and resolved against v2
clusters. A workspace group ID is not a cluster ID, so the spelling
parsed but could never match: every use of it missed, and the only
thing that made the miss legible was a hint appended to the error.
It now names what it says. IN GROUP carries its own group_id/group_name
placeholders and resolves through _get_stage_group() against v1, where
Stage is attached to the group itself -- stage/{group_id}/fs/ -- so a
group names a Stage with no workspace to add. A starter workspace is the
fallback for a name that is no group's, because it was reachable this
way before. Naming a group that does not exist raises: the clause says
which resource was meant, so there is nothing else to try.
A bare IN keeps working as it did and gains a second chance at a
workspace group, warning DeprecatedFeatureWarning when it takes it, so
statements written before Stage moved to v2 still resolve. The cluster
lookup goes first, so a name belonging to both stays the cluster's and
stays quiet.
_deployment_param's is-this-the-GROUP-spelling flag goes with the hint
it fed; _first_param takes the key paths as an argument instead, which
is what lets the two resolvers keep separate ones.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cleanup_deployments only answered "what did an old run strand": the age guard selects what is older than --older-than, and the name gate limits it to names the suite generates. Clearing out what today's sessions made needed the opposite of the first and none of the second. --since DATE replaces the age guard with a calendar cutoff -- today, yesterday or an ISO date, counted from local midnight, because the caller means their own calendar days. It replaces rather than stacks: both guards at once leaves a window nothing falls into. An unreported creation time is still spared, since it can be shown neither to be old enough nor to fall inside the window. --any-name drops the name gate. --kind restricts which resources are listed, and matters most here: --since and --any-name together remove both of the guards that keep this tool off deployments it did not create, and without --kind the same cutoff reaches every cluster of that age, the shared pool included. A kind that was not asked for is not even listed, so its API is never called. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three test-only fixes, no library change. TestStageFusion still spelled its owner clause IN GROUP [ID] '<cluster>' in _clear_stage and in the download and upload tests. IN GROUP now names a v1 workspace group, so a cluster ID can never match it and every test in the class died in tearDown with a KeyError. They use a bare IN, the v2 deployment spelling, which test_show_stage had already moved to. The starter user name is namespaced per run in both starter suites. It has to be unique across the project's starter deployments, not just within one: creating the same name in a second starter deployment fails while the first is live, and the API reports that with a bare 500 that names nothing. A fixed 'starter_user' therefore collided between test_management_v1's TestStarterWorkspace and test_management_v2's TestStarterCluster, which run on different xdist workers, and with whatever an earlier failed run leaked. There is no delete-user route, so uniqueness is the only lever; the deployments themselves are already tracked and swept. TestFusion gains coverage of the JOB write path's target. Nothing in the JOB grammar names a deployment, so targetID comes from the environment and targetType from the manager the handler picked, and neither is visible in any statement -- the pairing can only be observed in the POST jobs body, which the test asserts with the manager mocked at the request layer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A bare IN that matched no cluster already fell back to a workspace group, so that statements written before Stage moved to v2 keep resolving, but it warned when it did: "<name> is a workspace group, not a deployment ... name it with IN GROUP while it lasts, and a cluster with IN." That reads as a correction, and there is nothing to correct. Naming a group with a bare IN is what Stage statements always did -- a group was the only kind of Stage owner there was, and Stage is attached to the group itself at v1 -- so the spelling the user wrote is the one to keep writing. Whether it lands on a cluster or a group is a fact about their org, not about their SQL, and the migration the warning implied is not an edit to a statement. _group_fallback is therefore silent, and IN simply names either kind of Stage owner. This is the same reasoning _manage_workspaces_v1 exists for: an internal caller that is v1-only by design should not emit a warning the caller can do nothing about. IN GROUP still warns, because that spelling is going away with management/v1/ and dropping the keyword is an edit that works today either way. Its message said "Use IN <deployment> to name a cluster instead", which implied you needed a cluster before you could stop writing IN GROUP; it now points at a bare IN, which resolves both. Resolution order is untouched: IN GROUP still bypasses the cluster lookup, and a name belonging to both a cluster and a group is still the cluster's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
test_get_secret named its secret with id(self), a process-local address. The "clear a leftover secret from a previous run" block above it can therefore never match anything a previous run left, which made it dead code: a secret is org-scoped and permanent, nothing sweeps them, and the suite leaked one every time the test did not reach its own cleanup. Two such orphans had accumulated in the organization and have been removed by hand. The name is fixed again, as it is in the v1 suite, but distinct from that suite's 'secret_name' so the two do not delete each other's. TestSecrets holds a single test and no xdist_group, so there is never more than one instance of it per run for a fixed name to collide with. The cleanup path is now reachable, and was verified by planting a leftover and watching the test clear it. The ID for the final delete comes from the create response rather than from the get_secret call under test. Binding it inside the try left the finally raising UnboundLocalError over whatever the lookup had actually failed with -- which is exactly the shape of failure this test hits when the secrets service stalls, since that is upstream latency the SDK cannot retry a POST through. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TTLProperty subclassed object, so nothing outside its own __get__ could tell it apart from a method. Sphinx cannot: autodoc's PropertyDocumenter takes only property and functools.cached_property, so every member the decorator wraps was documented as a callable, and the published pages told readers to write manager.projects() -- five of them, across ClusterManager.projects, .regions, .shared_tier_regions and the two WorkspaceManager equivalents. api.rst already called them :attr:, so the reference disagreed with itself, and the spelling it shipped raises TypeError. Subclassing property fixes the rendering and is what the class always meant. The per-instance cache is untouched: __get__ reads and writes obj.__dict__ under _ttl_cache_<name>, a key distinct from the attribute, so data-descriptor precedence changes nothing about what it finds there. The getter is also held as _fget, because property.fget is read-only at runtime and Optional to mypy. One behavioural change, in the direction of the class's intent: assigning to one of these attributes now raises AttributeError rather than silently shadowing the descriptor with an instance attribute. Nothing assigns to one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
api.rst was broken, not merely stale: FilesObject moved out of management/workspace.py into management/files.py when the version-neutral code was level-set, and the autosummary entries still named the old module, so all fifteen of its pages built empty. Stage is documented next to a Cluster.stage that returns management.stage.Stage, but pointed at the v1 re-export path. A third entry offered StageObject.upload_file, which has never existed; the methods that return a FilesObject are FileSpace's. README and ARCHITECTURE still presented manage_workspaces() as the front door. README's example was worse than dated -- manager.workspaces.create() names an attribute WorkspaceManager does not have, so it cannot ever have run -- and its Fusion snippet spelled statements that now warn. Both are rewritten against v2, and each new statement was parsed through the registry first, which is how the WITH PASSWORD line came off CREATE CLUSTER: that clause is v1's, and the v2 grammar has no equivalent because the API generates the password. ARCHITECTURE's module tree predated the v1/v2 split entirely, its management diagram and class table named workspace.py for six classes that live elsewhere, and connection.py:1312 no longer lands on connect(). conf.py's intersphinx map had three URLs that answer 404 or 403 -- pandas pinned to a 0.19.2 tree, matplotlib at sourceforge -- so no cross-reference to any of them has resolved for some time. The build is warning-free again once they are current, which is what makes the two autosummary breakages above visible rather than lost in noise. The Fusion handler guide keeps its worked example, since it teaches grammar syntax rather than a vocabulary, but says which vocabulary it is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five docs under docs/ recorded work that is finished: the untwist plan whose Part 7 shipped, the fusion v2 cluster plan it unblocked, the wait-until-usable plan whose own header says all six steps landed, the branch review whose every item carries a resolution line, and the agent prompt for a round-trip stage that has landed. What they describe is readable from the code now, and their status annotations are one more thing to keep true. Three stay, because they are not records of finished work: the OpenAPI gap audit is mostly a list of fields the wrappers still do not carry, plus API facts established live that the spec dump does not state; the shared-deployment-pool plan holds an open question about what bounds concurrent provisioning now that deployment_slot() is gone; and stages 3 and 4 of the round-trip plan are unstarted. The five comments that cited a doc by path say their piece inline instead. Two of them wanted only a cross-reference and now name utils.shared_clusters. The pool comment in tests/utils.py was the one carrying real weight, so it spells out both halves of the rule: which classes must keep deploying their own and why -- a subject that gets PATCHed or terminated cannot be borrowed -- and what makes the four borrowers safe, which is that each scopes its assertions to itself. A handler docstring loses a pointer it should never have shown a user; the reason the FORCE clause is withheld was already stated beside it. Nothing under singlestoredb/ depends on a docs/ path any more, so a plan can be retired without breaking a comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
mgr.v2/entity.v2attribute syntaxmanagement/v1/andmanagement/v2/folders; top-level modules become thin re-export shims routing viaconfig.get_option('management.version')VersionedMixinproviding cached__getattr__-based version switching for both managers and entities, with credential cloning andfrom_dictreconstructionTest plan
test_versioned_management.pycovering:__getattr__pattern matching and cachingfrom_dict+ versioned managermanage_*()version routingManagementError)management.versionconfig option routing🤖 Generated with Claude Code
Note
High Risk
Changes the default cloud management API surface and provisioning behavior (projects, passwords, firewall waits), affecting anyone relying on v1 defaults or workspace-group workflows.
Overview
Makes Management API v2 the default (
management.version/SINGLESTOREDB_MANAGEMENT_VERSION,DEFAULT_MANAGEMENT_VERSION) and promotesmanage_clusters()with flatCluster/Project/StarterClusterresources, whilemanage_workspaces()and v1 workspace-group types remain available but deprecated.Introduces a versioned package layout (
management/v1/,management/v2/, shared top-level modules,_version_import/_resolve_version) so factories and helpers route by explicitversion=or the config option; v1-only callers (Fusion workspace grammar, inference helpers, test cluster scripts) pin v1 via_manage_workspaces_v1()orversion='v1'.manage_clusteris renamed/exported asmanage_clustersfrom the public package.Fusion SQL gains v2
CLUSTERhandlers (handlers/cluster.py), deprecates v1 workspace commands with warnings, retargets jobs/export/stage resolution to clusters where appropriate, and trims Stage/file upload round trips (_upload_local_file(..., fetch_info=False), lazyCluster.project/ region).CI and tests: default pytest-xdist (
-n 3 --dist loadgroup), force serial runs for HTTP/Data API jobs (-n 0), add pytest-xdist to wheel smoke tests, document shared-cluster pooling and management trace option; flake8 ignores for v1/v2 re-export modules.Reviewed by Cursor Bugbot for commit 087025e. Bugbot is set up for automated code reviews on this repo. Configure here.