Skip to content

fix(sqlite): report a selected database held by another process as retryable - #2911

Closed
pablodiazjorge wants to merge 5 commits into
morluto:mainfrom
pablodiazjorge:fix/sqlite-busy-selected-file
Closed

pablodiazjorge wants to merge 5 commits into
morluto:mainfrom
pablodiazjorge:fix/sqlite-busy-selected-file

Conversation

@pablodiazjorge

Copy link
Copy Markdown
Contributor

Summary

Fixes #2822 for the SQLite provider. On Windows, when another process holds the selected database open without read sharing, the read fails with EBUSY and inspect_sqlite_database reported a generic execution_failure with retryable: false, telling the caller to run rea doctor and not to retry. The same request succeeds once the other process releases the file.

Problem and expected behavior

Smallest trigger, on Windows: hold a database with [System.IO.File]::Open('<path>', 'Open', 'ReadWrite', 'None') and run rea inspect-sqlite-database <path> --json. Before this change: code: execution_failure, retryable: false. Expected: a retryable outcome that says the file is in use and how to recover.

Change and scope

  • src/sqlite/SqliteDatabaseFailures.ts: in the artifact-read phase, EBUSY now maps to AnalysisCapabilityUnavailableError with retryable: true, next to the existing EACCES/EPERM and ENOENT/ENOTDIR mappings. It projects as capability_unavailable, category unavailable, with a message to close the owning application or inspect a copy, and details.reason keeps the path and EBUSY. This is the same retryable unavailable route that fix(windows): preserve transient runtime admission failures #2467 uses for transient Windows sharing conflicts.
  • tests/boundary/sqlite/sqliteDatabaseBusy.test.ts: a portable test that injects the EBUSY open failure and checks the projection and that no private root is left, and a Windows-only test where a PowerShell process holds the file without sharing.

Scope: SQLite only. As #2822 notes, about 22 providers map EACCES/EPERM on selected-file reads and none maps EBUSY; a shared mapping is left for your decision there.

Contribution scope: not applicable (error classification within an existing provider).

Contract and boundary impact

  • Semantic owner and earliest changed stage: the SQLite provider's failure classification.
  • Contribution scope fit and linked maintainer decision: not applicable.
  • CLI and MCP/tool-catalog contract: no schema change; the error for this case changes from execution_failure to the existing capability_unavailable with retryable: true.
  • Provider, bridge, target-format, or platform compatibility: Windows behavior improves; POSIX hosts rarely return EBUSY for regular files.
  • Evidence, artifact, provenance, or reconstruction contract: none.
  • Target selection, authority, upstream behavior, and cleanup details (if applicable): no target is read; the private root is still cleaned up (checked in the test).
  • Generated metadata (docs/public/product-catalog.json), package, or installation impact: none.

Evidence and regression coverage

  • Tests added or updated: tests/boundary/sqlite/sqliteDatabaseBusy.test.ts (2 tests).
  • Base reproduction or other evidence: without the change, both tests fail with code: execution_failure; the real Windows case is in [Bug] Windows: a selected file held open exclusively by another process is reported as a non-retryable execution failure #2822.
  • User-visible CLI/MCP output (if applicable): {"code":"capability_unavailable","category":"unavailable","retryable":true,"message":"The selected SQLite database is held open by another process without read sharing. Close that application or inspect a copy, then retry.", ...}
  • Remaining proof gaps: macOS and Linux not run locally (the portable test covers the mapping there).

Validation performed

Windows 11 x64 (10.0.26300), non-elevated, Node 24.18.0, Windows native addon built from the same commit.

  • npx vitest run tests/boundary/sqlite/sqliteDatabaseBusy.test.ts on 1b6c899b0: 2 passed; without the change, both fail with code: execution_failure.
  • The SQLite test files: 96 passed, 9 skipped and 2 failed. The 2 failures are the Windows-only test assumptions fixed in test(sqlite): fix two SQLite boundary tests on native Windows #2824, not yet merged.
  • npm run check:fast: passed.
  • npx oxfmt --check on the two files: passed.

Compatibility, safety, and release

  • Breaking changes or migration steps: none; callers that retried only on retryable: true now retry this case.
  • Real Hopper/Ghidra, browser, or OS coverage: native Windows with a real sharing violation.
  • Package or release metadata impact: none.
  • Security, privacy, process, or containment review: the test spawns PowerShell only on Windows, to hold a file it created.

Before merge

  • This PR addresses a concrete problem or an agreed enhancement.
  • Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
  • The PR has one focused outcome and the title follows type(scope): outcome.
  • Related issue is linked, or the reason for not linking one is stated above.
  • Tests cover changed observable behavior and meaningful failure paths.
  • Owning docs, contracts, and generated metadata are updated where needed.
  • User-visible CLI/MCP changes include representative output.
  • I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 13, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-13T19:36:36.954747Z fe9aca2 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0e26710b4

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@greptile-apps

greptile-apps Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; the previous startup-error finding is fixed, and no new blocking issue remains.

Summary

The PR makes SQLite artifact-read failures with EBUSY retryable and names the affected database, WAL, or journal file.

  • SQLite reports a busy database as retryable and unavailable.

Reviews (5) · Last reviewed commit: "test(sqlite): fail fast when the busy-fi..." · Reviewed by Greptile

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5f6760beed

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/sqlite/SqliteDatabaseFailures.ts Outdated
return new AnalysisCapabilityUnavailableError(
SQLITE_PROVIDER_IDENTITY.id,
operation,
`Selected database file set is in use by another process (EBUSY): ${path}`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve the actual busy file-set member

When EBUSY comes from opening <path>-wal or <path>-journal (both are read by captureSqliteDatabaseSnapshot), sqliteDatabaseFailure still receives the caller's base database path and this reason substitutes it for the filesystem error's cause.path. The projected diagnostic therefore drops which member was actually locked, making it impossible to distinguish a busy database from a busy sidecar; preserve a validated cause.path here and fall back to the selected path when it is absent.

AGENTS.md reference: AGENTS.md:L13-L13

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 23057bf: the reason now names the member of the file set that EBUSY reports (the database, its WAL or its journal), validated against those three paths, with a test for a busy WAL.

@morluto morluto added area: runtime Process capture and runtime observation bug Something isn't working labels Oct 14, 2026
@kaoru0822-kitauji kaoru0822-kitauji added the ci-fix-wip Claimed by the CI-fix track label Oct 14, 2026
@kaoru0822-kitauji

Copy link
Copy Markdown
Collaborator

CI-fix track: this change is already on main. Merging current main into this branch yields a tree identical to main. Both src/sqlite/SqliteDatabaseFailures.ts and tests/boundary/sqlite/sqliteDatabaseBusy.test.ts match main exactly, landed in 5b7fe94 ("preserve extraction gaps and database failure reasons"), and the busy-database test passes there. The failing Static checks, analysisView message-count and productCatalog jobs reflected main's state at the time, not this PR. I didn't push an empty merge. This PR looks safe to close as superseded.

@kaoru0822-kitauji kaoru0822-kitauji removed the ci-fix-wip Claimed by the CI-fix track label Oct 14, 2026
@pablodiazjorge

Copy link
Copy Markdown
Contributor Author

Closing, since these changes landed on main in 5b7fe94, identical to this branch's head (fe9aca2): the EBUSY classification in SqliteDatabaseFailures.ts and sqliteDatabaseBusy.test.ts, including the holder start-failure fix. Thanks!

@morluto

morluto commented Oct 15, 2026

Copy link
Copy Markdown
Owner

Thanks @pablodiazjorge for distinguishing a temporarily locked SQLite database from malformed input. Main now reports the selected database as retryable while the competing process holds it (main).

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

Labels

area: runtime Process capture and runtime observation bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Windows: a selected file held open exclusively by another process is reported as a non-retryable execution failure

3 participants