feat: add Shipyard Neo file persistence option#8827
Open
hyperzlib wants to merge 2 commits into
Open
Conversation
dosubot
Bot
added
size:L
![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 file persistence support for Shipyard Neo sandboxes by mapping a persist_id to a cargo_id in the database. Key changes include database schema updates, SQLite implementation for persistence management, configuration metadata updates across multiple locales, and integration within the Shipyard Neo booter. The review feedback highlights three important issues: a potential IntegrityError in the SQLite upsert method due to missing duplicate checks, a risk of data loss/orphaned cargos from overly broad exception handling when verifying cargo existence, and a transaction management inconsistency in the delete method.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1875
to
+1885
| async def upsert_shipyard_neo_persist(self, persist_id: str, cargo_id: str) -> None: | ||
| """Create or update the persistent mapping for a Shipyard Neo cargo.""" | ||
| async with self.get_db() as session: | ||
| session: AsyncSession | ||
| async with session.begin(): | ||
| persist = ShipyardNeoPersist( | ||
| persist_id=persist_id, | ||
| cargo_id=cargo_id, | ||
| ) | ||
| session.add(persist) | ||
| await session.flush() |
Contributor
There was a problem hiding this comment.
upsert_shipyard_neo_persist 方法目前直接创建并使用 session.add(persist)，这在 persist_id 已经存在时会因为唯一约束（Unique Constraint）而抛出 IntegrityError 异常，无法实现真正的 "upsert"（更新或插入）逻辑。
在 _resolve_cargo_id 中，如果原有的 Cargo 在 Bay 上不存在（例如返回了 NotFoundError），系统会尝试重新创建 Cargo 并调用 upsert_shipyard_neo_persist，此时就会触发该唯一约束冲突导致崩溃。
建议先查询是否存在已有的记录，如果存在则更新 cargo_id，否则再进行插入。
async def upsert_shipyard_neo_persist(self, persist_id: str, cargo_id: str) -> None:
"""Create or update the persistent mapping for a Shipyard Neo cargo."""
async with self.get_db() as session:
session: AsyncSession
async with session.begin():
result = await session.execute(
select(ShipyardNeoPersist).where(
col(ShipyardNeoPersist.persist_id) == persist_id,
),
)
persist = result.scalar_one_or_none()
if persist is not None:
persist.cargo_id = cargo_id
else:
persist = ShipyardNeoPersist(
persist_id=persist_id,
cargo_id=cargo_id,
)
session.add(persist)
await session.flush()
Comment on lines
+628
to
+635
| except Exception as e: | ||
| logger.warning( | ||
| "[Computer] Error checking existing cargo for persist_id=%s: %s; " | ||
| "a new cargo will be created.", | ||
| self._persist_id, | ||
| e, | ||
| ) | ||
| cargo_id = None |
Contributor
There was a problem hiding this comment.
在检测已有的 Cargo 是否存在于 Bay 时，如果捕获到非 NotFoundError 的异常（例如临时网络抖动、超时或 Bay 服务端 500 错误），直接将 cargo_id 设为 None 会导致系统认为该 Cargo 不存在，从而在下方创建一个全新的空 Cargo 并覆盖数据库中的映射关系。这会导致用户原有的持久化数据丢失（旧的 Cargo 被孤立）。
建议对于非 NotFoundError 的异常，直接抛出（允许 boot 失败并让用户重试），或者至少保留原有的 cargo_id 继续尝试启动，而不是盲目创建新 Cargo。
Suggested change
| except Exception as e: | |
| logger.warning( | |
| "[Computer] Error checking existing cargo for persist_id=%s: %s; " | |
| "a new cargo will be created.", | |
| self._persist_id, | |
| e, | |
| ) | |
| cargo_id = None | |
| except Exception as e: | |
| logger.error( | |
| "[Computer] Error checking existing cargo for persist_id=%s: %s. " | |
| "Boot will fail to prevent data loss.", | |
| self._persist_id, | |
| e, | |
| ) | |
| raise |
Comment on lines
+1898
to
+1907
| async def delete_shipyard_neo_persist(self, persist_id: str) -> None: | ||
| """Delete the persistent mapping for a Shipyard Neo cargo.""" | ||
| async with self.get_db() as session: | ||
| session: AsyncSession | ||
| await session.execute( | ||
| delete(ShipyardNeoPersist).where( | ||
| col(ShipyardNeoPersist.persist_id) == persist_id, | ||
| ), | ||
| ) | ||
| await session.commit() |
Contributor
There was a problem hiding this comment.
delete_shipyard_neo_persist 方法中直接使用了 await session.commit()，而没有像该文件中的其他删除/修改方法一样使用 async with session.begin(): 来管理事务。为了代码的一致性和事务安全性，建议使用 async with session.begin(): 块。
Suggested change
| async def delete_shipyard_neo_persist(self, persist_id: str) -> None: | |
| """Delete the persistent mapping for a Shipyard Neo cargo.""" | |
| async with self.get_db() as session: | |
| session: AsyncSession | |
| await session.execute( | |
| delete(ShipyardNeoPersist).where( | |
| col(ShipyardNeoPersist.persist_id) == persist_id, | |
| ), | |
| ) | |
| await session.commit() | |
| async def delete_shipyard_neo_persist(self, persist_id: str) -> None: | |
| """Delete the persistent mapping for a Shipyard Neo cargo.""" | |
| async with self.get_db() as session: | |
| session: AsyncSession | |
| async with session.begin(): | |
| await session.execute( | |
| delete(ShipyardNeoPersist).where( | |
| col(ShipyardNeoPersist.persist_id) == persist_id, | |
| ), | |
| ) |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
upsert_shipyard_neo_persistimplementation always inserts a newShipyardNeoPersistrow despitepersist_idbeing unique, which will raise integrity errors on reuse; consider doing aSELECT+ update or using anON CONFLICT/merge-style pattern to truly upsert. - In
_resolve_cargo_id, the broadexcept Exceptionblocks (especially aroundcargos.getandcargos.create) may hide genuine bugs or misconfigurations; it would be safer to catch more specific Shipyard Neo exceptions and let unexpected errors surface.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `upsert_shipyard_neo_persist` implementation always inserts a new `ShipyardNeoPersist` row despite `persist_id` being unique, which will raise integrity errors on reuse; consider doing a `SELECT` + update or using an `ON CONFLICT`/`merge`-style pattern to truly upsert.
- In `_resolve_cargo_id`, the broad `except Exception` blocks (especially around `cargos.get` and `cargos.create`) may hide genuine bugs or misconfigurations; it would be safer to catch more specific Shipyard Neo exceptions and let unexpected errors surface.
## Individual Comments
### Comment 1
<location path="astrbot/core/db/sqlite.py" line_range="1875-1884" />
<code_context>
+ # ====
+
+ @abc.abstractmethod
+ async def upsert_shipyard_neo_persist(self, persist_id: str, cargo_id: str) -> None:
+ """Create or update the persistent mapping for a Shipyard Neo cargo."""
+ ...
</code_context>
<issue_to_address>
**issue (bug_risk):** The `upsert_shipyard_neo_persist` implementation will fail on duplicate `persist_id` instead of updating the existing row.
This currently always creates and adds a new `ShipyardNeoPersist`, so a second call with the same unique `persist_id` but different `cargo_id` will raise an integrity error instead of updating. Please either look up and update the existing row first, or use an `ON CONFLICT`-style upsert so repeated calls with the same `persist_id` truly behave as an upsert.
</issue_to_address>
### Comment 2
<location path="astrbot/core/computer/booters/shipyard_neo.py" line_range="621-627" />
<code_context>
+ # Detect if the cargo exists on Bay
+ try:
+ await self._client.cargos.get(cargo_id)
+ except NotFoundError:
+ logger.info(
+ "[Computer] No existing cargo found on Bay for persist_id=%s; "
+ "a new cargo will be created.",
+ self._persist_id,
+ )
+ cargo_id = None
+ except Exception as e:
+ logger.warning(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Consider cleaning up stale DB mappings when the cargo no longer exists on Bay.
In the `NotFoundError` branch you log and clear `cargo_id`, but the `ShipyardNeoPersist` row still points to the deleted cargo, leaving a stale mapping. To keep Bay and the DB consistent and avoid accumulation of stale rows, consider deleting that `ShipyardNeoPersist` entry (e.g., `delete_shipyard_neo_persist(self._persist_id)`) or otherwise updating it here.
Suggested implementation:
```python
# Detect if the cargo exists on Bay
try:
await self._client.cargos.get(cargo_id)
except NotFoundError:
logger.info(
"[Computer] No existing cargo found on Bay for persist_id=%s; "
"the stale DB mapping will be removed and a new cargo will be created.",
self._persist_id,
)
if self._db_helper is not None:
await self._db_helper.delete_shipyard_neo_persist(self._persist_id)
cargo_id = None
except Exception as e:
logger.warning(
```
If `BaseDatabase` / the concrete DB helper does not yet implement `delete_shipyard_neo_persist(persist_id: str)`, you should:
1. Add an async `delete_shipyard_neo_persist` method to `BaseDatabase` (and any subclasses) that deletes the row for the given `persist_id`.
2. Ensure all calls to this new method are awaited, as shown in the edit above.
</issue_to_address>
### Comment 3
<location path="astrbot/core/computer/booters/shipyard_neo.py" line_range="600" />
<code_context>
return chosen
+
+ async def _resolve_cargo_id(self) -> str | None:
+ if self._persist_id is None:
+ return None
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring `_resolve_cargo_id` into a short orchestration method that delegates DB lookups, Bay existence checks, cargo creation, and mapping persistence to focused helpers to reduce branching and nesting.
You can keep all the new behavior but simplify `_resolve_cargo_id` by:
- Guard‑clause precondition checks.
- Extracting the SDK-compat creation logic into a helper.
- Extracting DB mapping logic into a helper.
- Making `_resolve_cargo_id` a short orchestration function with minimal branching.
For example:
```python
async def _resolve_cargo_id(self) -> str | None:
if not self._persist_id:
return None
if self._db_helper is None:
logger.warning(
"[Computer] persist_id is set but no db_helper provided; "
"file persistence will not work."
)
return None
cargo_id = await self._load_persisted_cargo_id()
if cargo_id:
cargo_id = await self._ensure_cargo_exists_on_bay(cargo_id)
if not cargo_id:
cargo_id = await self._create_cargo_with_compat_fallback()
if cargo_id:
await self._save_cargo_mapping(cargo_id)
return cargo_id
```
Then keep each responsibility in a small helper:
```python
async def _load_persisted_cargo_id(self) -> str | None:
ret = await self._db_helper.get_shipyard_neo_persist(self._persist_id)
return ret.cargo_id if ret is not None else None
```
```python
async def _ensure_cargo_exists_on_bay(self, cargo_id: str) -> str | None:
try:
await self._client.cargos.get(cargo_id)
return cargo_id
except NotFoundError:
logger.info(
"[Computer] No existing cargo found on Bay for persist_id=%s; "
"a new cargo will be created.",
self._persist_id,
)
except Exception as e:
logger.warning(
"[Computer] Error checking existing cargo for persist_id=%s: %s; "
"a new cargo will be created.",
self._persist_id,
e,
)
return None
```
```python
async def _create_cargo_with_compat_fallback(self) -> str | None:
try:
try:
cargo = await self._client.cargos.create()
except BayError:
# 旧版 SDK 需要传入 size_limit_mb，暂时固定为 10GB
cargo = await self._client.cargos.create(size_limit_mb=10240)
return cargo.id
except Exception as e:
logger.error(
"[Computer] Failed to create cargo for persist_id=%s: %s; "
"file persistence will not work.",
self._persist_id,
e,
)
return None
```
```python
async def _save_cargo_mapping(self, cargo_id: str) -> None:
await self._db_helper.upsert_shipyard_neo_persist(self._persist_id, cargo_id)
logger.info(
"[Computer] Created new cargo for persist_id=%s: cargo_id=%s",
self._persist_id,
cargo_id,
)
```
This keeps all existing behavior (persistence, compatibility fallback, logging) but:
- Reduces nesting and stateful reassignments.
- Separates DB concerns from SDK concerns.
- Makes the orchestration flow in `_resolve_cargo_id` easy to read and test.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+621
to
+627
| except NotFoundError: | ||
| logger.info( | ||
| "[Computer] No existing cargo found on Bay for persist_id=%s; " | ||
| "a new cargo will be created.", | ||
| self._persist_id, | ||
| ) | ||
| cargo_id = None |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Consider cleaning up stale DB mappings when the cargo no longer exists on Bay.
In the NotFoundError branch you log and clear cargo_id, but the ShipyardNeoPersist row still points to the deleted cargo, leaving a stale mapping. To keep Bay and the DB consistent and avoid accumulation of stale rows, consider deleting that ShipyardNeoPersist entry (e.g., delete_shipyard_neo_persist(self._persist_id)) or otherwise updating it here.
Suggested implementation:
# Detect if the cargo exists on Bay
try:
await self._client.cargos.get(cargo_id)
except NotFoundError:
logger.info(
"[Computer] No existing cargo found on Bay for persist_id=%s; "
"the stale DB mapping will be removed and a new cargo will be created.",
self._persist_id,
)
if self._db_helper is not None:
await self._db_helper.delete_shipyard_neo_persist(self._persist_id)
cargo_id = None
except Exception as e:
logger.warning(If BaseDatabase / the concrete DB helper does not yet implement delete_shipyard_neo_persist(persist_id: str), you should:
- Add an async
delete_shipyard_neo_persistmethod toBaseDatabase(and any subclasses) that deletes the row for the givenpersist_id. - Ensure all calls to this new method are awaited, as shown in the edit above.
|
|
||
| return chosen | ||
|
|
||
| async def _resolve_cargo_id(self) -> str | None: |
Contributor
There was a problem hiding this comment.
issue (complexity): Consider refactoring _resolve_cargo_id into a short orchestration method that delegates DB lookups, Bay existence checks, cargo creation, and mapping persistence to focused helpers to reduce branching and nesting.
You can keep all the new behavior but simplify _resolve_cargo_id by:
- Guard‑clause precondition checks.
- Extracting the SDK-compat creation logic into a helper.
- Extracting DB mapping logic into a helper.
- Making
_resolve_cargo_ida short orchestration function with minimal branching.
For example:
async def _resolve_cargo_id(self) -> str | None:
if not self._persist_id:
return None
if self._db_helper is None:
logger.warning(
"[Computer] persist_id is set but no db_helper provided; "
"file persistence will not work."
)
return None
cargo_id = await self._load_persisted_cargo_id()
if cargo_id:
cargo_id = await self._ensure_cargo_exists_on_bay(cargo_id)
if not cargo_id:
cargo_id = await self._create_cargo_with_compat_fallback()
if cargo_id:
await self._save_cargo_mapping(cargo_id)
return cargo_idThen keep each responsibility in a small helper:
async def _load_persisted_cargo_id(self) -> str | None:
ret = await self._db_helper.get_shipyard_neo_persist(self._persist_id)
return ret.cargo_id if ret is not None else Noneasync def _ensure_cargo_exists_on_bay(self, cargo_id: str) -> str | None:
try:
await self._client.cargos.get(cargo_id)
return cargo_id
except NotFoundError:
logger.info(
"[Computer] No existing cargo found on Bay for persist_id=%s; "
"a new cargo will be created.",
self._persist_id,
)
except Exception as e:
logger.warning(
"[Computer] Error checking existing cargo for persist_id=%s: %s; "
"a new cargo will be created.",
self._persist_id,
e,
)
return Noneasync def _create_cargo_with_compat_fallback(self) -> str | None:
try:
try:
cargo = await self._client.cargos.create()
except BayError:
# 旧版 SDK 需要传入 size_limit_mb，暂时固定为 10GB
cargo = await self._client.cargos.create(size_limit_mb=10240)
return cargo.id
except Exception as e:
logger.error(
"[Computer] Failed to create cargo for persist_id=%s: %s; "
"file persistence will not work.",
self._persist_id,
e,
)
return Noneasync def _save_cargo_mapping(self, cargo_id: str) -> None:
await self._db_helper.upsert_shipyard_neo_persist(self._persist_id, cargo_id)
logger.info(
"[Computer] Created new cargo for persist_id=%s: cargo_id=%s",
self._persist_id,
cargo_id,
)This keeps all existing behavior (persistence, compatibility fallback, logging) but:
- Reduces nesting and stateful reassignments.
- Separates DB concerns from SDK concerns.
- Makes the orchestration flow in
_resolve_cargo_ideasy to read and test.
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.
实现Shipyard Neo的Cargo支持，以方便持久化Sandbox中的数据
Modifications / 改动点
添加
shipyard_neo_persist表，在配置文件中增加shipyard_neo_persist_id以绑定 Cargo 容器由于 Shipyard Neo 那边的 Cargo 不支持自定义 id，所以创建了一张映射表，用户填写
shipyard_neo_persist_id后，创建 Sandbox 时会先从数据库查询有没有与 persist_id 对应的 cargo id，如果不存在则调用 Shipyard Neo 的 SDK 创建 Cargo，并记录与 persist_id 的映射关系Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能，已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试，并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml.😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Add support for persistent Shipyard Neo sandboxes by mapping logical persistence IDs to Cargo volumes and wiring this into the Shipyard Neo booter and configuration.
New Features:
Enhancements:
Documentation: