Repository navigation
fix(browse): report an import job Studio stopped following as outcome unknown, not failed - #1955
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces ImportJobUnconfirmedError to handle scenarios where Studio stops following a database import job due to consecutive polling failures or reaching the timeout deadline. This ensures that unconfirmed imports are treated as having an unknown outcome rather than being reported as failures, preventing accidental retries that could duplicate data. The global query client error handling has been updated to support this new error type, and corresponding unit tests and design documentation have been added. There are no review comments, so I have no feedback to provide.
dawsontoth
marked this pull request as ready for review
October 14, 2026 16:42
dawsontoth
marked this pull request as draft
October 14, 2026 16:51
dawsontoth
marked this pull request as ready for review
October 14, 2026 18:06
… unknown, not failed Once Harper accepts a CSV load, `waitForJob` follows its job until COMPLETE or ERROR. It also stopped after five failed get_job polls in a row (rethrowing the poll's error) and at its 10-minute deadline (a plain "Timed out" error), and the global mutation toast showed both as an ordinary failure. The load may still be running or may have finished, and a retry of a CSV with no primary-key column adds every row a second time. Both now reject with `ImportJobUnconfirmedError`, carrying the job id and why following stopped (a deadline that passes on a failed poll is reported as that poll's failure, with it as the cause). `describeError` shows it under #1897's "Outcome unknown" title with copy that points to the table, warns that importing again may add the rows twice, and names the job on the Status page's Jobs tab for a role that can list jobs. A job Harper finished with ERROR is still an `ImportJobError` failure. `describeError`, `shouldToastOwnFailure` and the toast's never-collapse rule now read one `describeUnknownOutcome` list (unanswered write, unconfirmed restart, unconfirmed import job), so a kind added there reaches all three. Closes #1939 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ve, after 20 seconds On a cluster reached through its own address, a `get_job` poll can land on a node other than the one that ran the import. `hdb_job` does not replicate, so that node answers `[]`, and Studio kept polling until the 10-minute deadline. Studio cannot poll the right node instead: the load answers only `message` and `job_id`, the job row records no node, and `get_job` reads only the local table. `waitForJob` now rejects with `ImportJobUnconfirmedError` (reason `not-found`) once `get_job` has answered `[]` for 20 seconds straight. A poll that finds the job, or one that fails, starts the grace again. The copy says the same as the other unknown outcomes, and points a role that can list jobs to the Jobs tab of each instance's Status page, since only the node that ran the job lists it. The grace is now one constant, `JOB_NOT_FOUND_GRACE_MS`, shared with the rolling-deploy follower, which already applied the same 20 seconds for the same reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…, by design A load whose job ends in ERROR after the last poll Studio could read is reported as an unknown outcome, not a failure: Studio can't report an outcome it never observed. Say so in the databases design note, so it reads as the contract it is. Drops the class JSDoc on ImportJobUnconfirmedError and trims waitForJob's to its rejection contract: both repeated the design note. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`waitForJob` read `Date.now()` in the failed-poll branch and again at the deadline check. A poll that failed just under the deadline could pass the first read and fail the second, and was then reported as `deadline` with no cause instead of `polling-failed` with the poll's error. Datadog then kept a timed-out poll it filters everywhere else. One read per poll makes the checks agree. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…unt moves The row editor's Next probes the next page past a full unfiltered page, and an empty answer is remembered (knownLastPage) so the dead probe isn't offered again. Only the view's own Refresh retired that proof, so after a window-focus refetch or an import's refresh Next stayed disabled on the last record even when records had been added. The proof is about how many records the table holds, so it now carries the record count the step set out with and holds only while the count is the same. A focus refetch or an import's refresh reads describe_table again; a count that moved voids it. A probe whose count moved while it was out proves nothing, so it neither stores a proof nor says "This is the last record". Reading a page doesn't, so the read on the way back from the probe keeps the proof however slow the probe. A step that turns the page now first drops the table's cached pages that aren't on screen, so it reads the next one afresh. Otherwise Next could reuse the probe's cached empty answer, or join a read of the page that started before the change, and prove the old last page again; and a step could open a row from a copy older than the page on screen. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…save each file on its own A save settled in its `mutate()` callback, which TanStack Query drops once the page unmounts, so a save that landed after you left kept the file marked unsaved and its stale copy could be saved back later. It now settles in the mutation's own `onSuccess`, from the request alone, and cancels a read of the file that was already in flight so it can't put the old contents back. A save that lands after sign-out cleared the caches settles nothing. The cached size becomes the UTF-8 length of what was written, and the cached time is dropped, since set_component_file reports neither. The .env secrets view's whole-file write now goes through the same save (`writeFile`, which rejects instead of toasting), so it is tracked, refused like any other, and settles after the page is gone. "Saving" was one flag for the whole page, so Save on another file did nothing while one saved. Saves are now tracked per file in the mutation cache, so another file stays editable and saves alongside, and a second save of the same file is refused with a toast when it held newer changes. A visual schema put back to the saved one during a save stays unsaved, so the save landing no longer replaces it. A save and a rename or move of an instance's files no longer overlap: a save that landed after the rename read the file dropped the saved text with the original. `useRenameFiles` refuses while an editor save is in flight, `saveFile` refuses while a rename is, and the Rename dialog is disabled during a save, so F2 and the tree's menu wait like the toolbar did. A folder rename holds off saves until its old folder is dropped too. Closes #1945 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aced with the same text The text editor dropped a model's history only when the text it was handed changed without being typed. Replacing the open file with the text it already showed (type B, save, upload B) changed nothing it was handed, and the reload can even land before the provider shows the contents missing. So Ctrl+Z brought back the earlier text and Save wrote it over the upload. editorPathState now counts each delete, replace and move away per path, and the text editor clears the attached model's history once the contents after a counted replacement are loaded, whatever the text. It tracks per model which file and count the history follows, so a switch to another file and back before the old model is disposed still clears it, and the same path on another instance, which shares the model, no longer undoes into this instance's file. Typing, saving and switching files keep their history. The editor also subscribes to its file's query for its read-only check: a reload that brings back the text the provider already holds changed nothing else it rendered from, which could leave it read-only. The test double now applies a `value` to a read-only editor with `setValue` even when the text is unchanged, as @monaco-editor/react 4.7.0 does, and a test on real Monaco pins that `setValue` with unchanged text drops the model's undo history. Closes #1951 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Browse asked four questions with window.confirm(): before a filter no index can answer, before Cleanup Orphan Blobs, before Delete Selected, and before the row editor's Previous/Next discards an unsaved draft. Each now asks in an AlertDialog. The full-scan one names the table(s) and the unindexed columns, with Scan anyway and Cancel. Nothing blocks while a dialog is open, so each question is a pending request holding what it would act on (the filter's conditions, the selected keys, the step's direction). In DatabaseTableView it also holds the view's context (instance, database, table, deep-link filters): the view stays mounted when the route moves to another table, and a request from elsewhere is cleared, not answered. The editor's question is cleared once its draft is gone, and Delete Selected's once the rows it named leave the screen. A yes clears the request and runs it once; Cancel or Escape leaves everything as it was. Applying or declining a filter now resets every filter input it doesn't name to empty. Left out of the reset, an input fell back to an earlier filter's value, which a declined deep link then sent with the next Apply. A filter an index can answer still applies at once, with no dialog and no change to applyFilters' synchronous shape. fullScanWarning becomes fullScanQuestion, which returns a title and a description. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Cleanup Orphan Blobs is disabled while a cleanup is running, but its handler was on onClick, which Radix runs even for a disabled menu item; only CSS kept a pointer off it. A screen reader's activation or a synthetic click therefore still asked to start a second cleanup. The handler moves to onSelect, which Radix gates on disabled, as #1953 did for Export CSV. No other disabled-capable menu item under src/features/instance/databases uses onClick. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The tree and overview menus offered Drop Table whenever the database had more than one table, even for a table a refetch had just dropped from the map (another tab or user removed it while its menu was open). Like Add Records, Import and Export CSV since #1953, it now waits for the map to hold the table. The menu reads the map's own keys, so a table named `constructor` no longer finds Object.prototype's and counts as present. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ct member resolveDatabasesRedirect sends the view away from a database or table the map no longer holds, and that redirect is what keeps the table view's toolbar from offering Drop Table for a dropped table. It looked the names up with a plain property read, so a table named `constructor` or `toString` found Object.prototype's member, counted as present, and the view stayed on a table that was gone. It now checks own keys. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Browse looked tables and databases up in the describe_all map and the schema-relationship map with plain property reads, so a name such as `constructor` or `toString` found Object.prototype's member. Import Data then took a new table called `constructor` for an existing one and said nothing about creating it, and a relationship lookup could be handed a function where it expected an array. describedTable.ts adds ownEntry, describedDatabase, describedTable and declaredRelationships, which read own keys only. The table menus, the table view, the overview, Import Data, Add Records, the CSV export, the relationship lookup and resolveDatabasesRedirect now go through them. The redirect also treats a database entry with no describe as missing instead of throwing on it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
parseSchemaRelationships filled plain objects keyed by database and table names read from the instance's component schema files. A database named `__proto__` therefore wrote its tables onto Object.prototype, and one named `constructor` wrote them onto Object itself. Both levels of the map are now null-prototype objects, where those names are ordinary keys, and the query turns off structural sharing, which would rebuild a changed level as a plain object on the next refetch. ownEntry also returns undefined, not null, for an entry that holds nothing. The databases tree also reads each database's tables through describedDatabase, so every by-name read of the describe map in Browse goes through the own-key helpers, as DESIGN.md says. Listing works the same way: the tree, the overview, the redirect's fallback and both "last table" checks use describedDatabaseNames and describedTableNames, which name only entries that have a describe, so they agree with the lookups on an entry that has none. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… servers, then forget them A self-hosted cluster's servers may outlive its removal from Fabric, so #1954 kept its remembered sign-ins rather than forget them with no logout. Removing one from Studio now ends those sessions instead. authStore.signOutEntities(ids) starts one best-effort logout per remembered id, built the way app sign-out builds them (the remembered credential, a 10s timeout, no response interceptors), forgets every id locally whatever the servers answer, and never rejects. terminateCluster calls it for a self-hosted cluster's own sign-in and each instance's once the DELETE succeeds, without making the removal wait. A Fabric cluster's terminate is unchanged, and a removed self-hosted cluster is still never cleared on render. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
onInstanceLogoutSubmit clears the entity's Basic Auth credential and Fabric Connect state once the logout answers. App sign-out and cluster removal forget the entity before they send, so by the time a late answer lands, anything recorded for it is a newer sign-in, and the clear wiped it: a new Basic Auth credential (from this tab or another), a new Fabric Connect flag, or the token of a Fabric Connect still verifying. sendBestEffortLogout now snapshots what the answer would clear when it sends: the stored Basic Auth credential and Fabric Connect flag, read raw from localStorage so another tab's writes count, and the connection generation, which moves when a Fabric Connect token is claimed. It hands onInstanceLogoutSubmit a signedInSince check, which skips the clear once the snapshot has changed and clears again otherwise. Explicit sign-outs pass no check and clear as before. signOutEntities also reads the sign-in markers from storage, not only this tab's copy, so removing a self-hosted cluster logs out a sign-in another tab made after this one loaded, and drops that sign-in's marker from storage too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…arkers Each tab read Studio:PotentiallyAuthenticated once at startup and wrote that whole copy back on every sign-in and sign-out. That erased every marker another tab had added since. An app sign-out from the stale tab then sent those sign-ins no logout, and a sign-out of an entity only another tab held left its marker behind. - flagKeyAsSignedIn and flagKeyAsSignedOut now change one entity in the record as stored now, and leave the rest alone. A sign-in updates this tab's copy only once its write lands, so a refused write is retried at the next sign-in. - A passing check of a sign-in this tab already marked still writes nothing, and a failing one removes the marker only when it still holds the URL it checked. Neither undoes another tab's change. - When storage refuses to take a signed-out entity out of the record, this tab's later writes leave it out until one lands, instead of trusting or discarding the whole record. - App sign-out, the local full sign-out and signOutEntities act on this tab's markers and every stored one, and log out each URL an entity is marked at, since another tab may have signed in again at a newer one. - signOutEntities' forgetStoredMarkers is gone: flagKeyAsSignedOut now drops another tab's marker for the entity it signs out, so managed cluster termination, which signs out locally, reaches it too. - A stored record that is not an object is treated as unreadable. - DESIGN.md records what remains: storage has no atomic read-modify-write, so two tabs writing at once can still lose one change, and a marker removed elsewhere stays removed. Closes #1968 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The markers lived in one shared Studio:PotentiallyAuthenticated record, so every write read and rewrote the whole of it. Two tabs writing at once could still lose a change, a refused write could take every marker with it, and a full sign-out rewrote the record once per entity. - Each marker now has its own key, Studio:PotentiallyAuthenticated:<id>, holding the URL signed into. A sign-in or sign-out touches only its entity's key, and sweeps enumerate the keys by prefix. - On load, the legacy record is copied into per-entity keys once per storage, and a marker already stored per entity wins. A record that is not one (an array, or not JSON) is removed at every load, since the previous release cannot start on it. - For one release, every marker change is mirrored into the legacy record, and sign-out sweeps read it. A tab still on the previous release, or a rollback, then still sees this release's sign-ins and sign-outs, and an app sign-out here reaches a sign-in only such a tab recorded. The copy is never repeated: that tab writes its whole load-time record back, which would bring back what this release signed out. - A sign-in storage refuses to record no longer throws out of setUserForIdAndKey or a check. Its next sign-in or check records it. A marker storage would not remove is retried at every marker write. - A check's answer is dropped when this tab signed in again at another URL while the check was out. - Unit tests seed and read markers through signInMarkersTestHelpers, and the e2e anon specs through e2e/tests/signInMarkers.ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ng silently - The sign-out wipe keeps the migration flag, so a previous-release tab writing its record back after a full sign-out isn't migrated again. - Until a migration lands, as when a full quota refuses it, the legacy record also seeds this tab's markers, so its sign-ins still check out. - Migration decides from one read, and also drops entries that aren't markers from the legacy record. A sign-in leaves a legacy record it can't read alone instead of replacing other tabs' markers. - A sign-in always records itself, so signing in again restores a marker another tab removed; only a check's passing answer skips it. - A check's answer is dropped when this tab signed in again while it was out, at the same URL or another, using a per-entity sign-in revision. - A marker storage refused to remove is retried only while it still holds the value it had, so another tab's sign-in there survives, and a marker that couldn't be read at sign-out is still removed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- A failed check is a probe, not a sign-out: when it can't read the marker it leaves it, and a mirror write it can't make no longer takes the whole legacy record. - For the app sign-in, which every /User/current answer re-signs, those answers no longer write the marker, so they can't put back one another tab's failed check dropped. - A failing app check that was out during a new app sign-in is dropped, as an entity's already was. - This tab's own sign-in cancels a removal of its marker still owed from a refused write. - Migration checks its flag before the best-effort cleanup, so a refused cleanup on a migrated storage can't seed signed-out entities. The sign-out wipe marks the storage migrated, as nothing of the legacy record survives it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A page-load app check still out when the user signed in again applied its answer for the old session over the new sign-in: getCurrentUser synced it into the store, and loadUser applied it again. - The store's app check now fetches through fetchCurrentUser, which doesn't sync, so loadUser applies the answer itself, and the sign-in revision guard drops a stale passing answer as it drops a failing one. - getCurrentUser syncs its answer only if no sign-in landed while it was in flight, using the store's new getSignInRevision. - DESIGN.md links #1970 for removing the legacy mirror. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- getCurrentUser syncs through the new syncAppUser, which writes no marker this tab already holds and leaves the sign-in revision alone. A sync landing during the store's own app check therefore no longer makes that check drop its answer and skip its sign-out re-ask. - setUserForIdAndKey treats every sign-in, the app's included, as one that always records itself. - A removal storage still owes is cancelled up front only when the kept marker already holds the URL being signed in, and otherwise once the new marker is written. - A migration flag that can't be read is reported as migrated, so a previous-release write-back can't seed that load. - getSignInRevision sits under its own doc comment. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dawsontoth
force-pushed
the
claude/1939-import-outcome-unknown
branch
from
October 15, 2026 13:33
eb2006b to
1f93df1
Compare
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
1 participant
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.
🪢 Folded in
This PR now carries seven more PRs, so one Rebase and merge lands all of them. Each was reviewed, and its gemini threads resolved, at its own head. Their commits are replayed here in merge order onto
stage0b48a411c, with no conflicts, as 21 signed commits and no merge commits. GitHub shows each as merged into this branch:confirm()Object.prototypeChecked at the top,
e7ae19ff7:npx tsc -b,npx oxlintandnpx dprint checkexit 0, andnpx vitest run --maxWorkers=4runs 566 files with 5,855 tests passing. The review footer below covers this PR's own commits; each folded PR's footer covers its own.Closes #1940
Closes #1945
Closes #1951
Closes #1924
Closes #1968
⊙ Problem
After Harper accepts a CSV import, Studio follows its job until it reports
COMPLETEorERROR. Studio also called the import failed after five failed status checks in a row, or after 10 minutes. On a cluster, a check can reach a node without the job, which kept Studio waiting all 10 minutes. The load may still be running, or done. Retrying a CSV with no primary-key column adds every row again. Closes #1939.🤔 Is this the right size?
As #1939 asks, only the error and its wording change. The modal still returns to its form with Import enabled. The toast fades after sonner's default 4 seconds, as for #1897's unanswered submission. A sticky toast or a locked modal would change both cases, so I left that for a decision.
💡 Solution
ERRORis still a failure, in Harper's words.⚖️ Alternatives
new Error('Outcome unknown: …'): the job id would go intoerror.message, splitting Error Tracking groups.ERRORthat came after the last poll Studio could read as a failure: Studio can't report an outcome it never observed, so it reads as unknown. Accepted by design; no follow-up.Why Studio can't poll the node that ran the job (Harper v5.3.2)
messageandjob_id(server/serverHelpers/serverUtilities.ts:581-584).server/jobs/JobObject.ts:10-19).get_jobreads only the localhdb_job(server/jobs/jobs.ts:38-54,:280-290).hdb_jobdoes not replicate (resources/databases.ts:330-339).fqdn(getOperationsUrlForCluster), which can reach any node. Central manager's proxy sends every request to the first running instance (central-managersrc/resources/cluster/Cluster.js:375-380), so it keeps reaching one node unless instance states change.🤔 Are 20 seconds and 10 minutes the right limits?
The 20-second grace for a missing job is the rolling-deploy follower's, and it applies after the job was seen too. The 10-minute deadline is unchanged, because the modal can't be closed while it waits. Both are one constant.
🔧 Changes
getJob.ts: the newImportJobUnconfirmedErrorand its copy, with a not-found hint that points to each instance's Jobs tab.waitForJob's contract. Five failed polls, 20 seconds of[]and the deadline all reject with it. A found job or a failed poll restarts the grace. One clock read per poll means a poll that fails at the deadline is reported as that failure, with it as the cause, so Datadog filters it the same way.JOB_NOT_FOUND_GRACE_MSmoves out ofrestartJob.ts, which now imports it with the same 20-second value.queryClient.ts: onedescribeUnknownOutcomelist for every unknown-outcome error, read bydescribeError,shouldToastOwnFailureand the toast's never-collapse rule. Restart and unanswered-write behavior is unchanged: neither error carries the code or status that would collapse it.DESIGN.md: the index entry and one list of unknown-outcome errors. The databasesDESIGN.md: the invariant that only anERRORStudio read is a failed import, and why the polled node can lack the job, with the Harper citations.✅ Verification
ImportDataModal.outcome.test.tsx: the real modal, mutation, poll loop and global error handler. The instance's answers are scripted, and the toast library, router and auth are stubbed. It fails on base with titleErrorfor the first two stops. The missing-job case fails on the previous head (still "Importing…" at 30 seconds). A job that ends inERRORstill toasts Harper's message.getJob.test.tspins the deadline cases, the not-found stop, its reset, a failed poll at the deadline, the single clock read and Datadog filtering parity.queryClient.test.tspins the copy and the no-collapse rule.importData.test.tsxchecks each outcome's error type.tsc -b,oxlint,dprint check,vite buildand the full vitest suite (553 files, 5,512 tests) pass.🤖 Generated by Claude Code (Claude Opus 5.5); posted via @dawsontoth.
Related PRs: #1897 overlaps (extends its unknown-outcome branch in describeError), #1899 overlaps (folds RestartUnconfirmedError into the same list), #1868 overlaps (its restart-job follower now shares the not-found grace), 5 others independent
Complexity: medium
Deferred findings
1ebd2884610ec52cnit correctness —src/integrations/api/instance/database/getJob.ts— server-side ERROR missed between last good poll and five failures reads as outcome unknown, not failure — filed Import: re-check an unknown import outcome once Studio can read the job again #1957Review-Coverage: authored=claude; ran=cursor-composer,gemini,codex,cursor-muse; adjudicated=domain; declined=cursor-grok,cursor-kimi,muse; rounds=9; full=4 @ eb2006b
Review-Attention: read ~4m (decisions: unseen-error-reads-unknown, not-found-grace-20s, import-button-stays-enabled, shared-grace-constant-home, do-less-alternative, not-found-grace-after-sighting, deterministic-get-job-refusal) @ eb2006b