fix: P0-5 — upload-status rollback, streaming copy (LOCAL only), user re-verify, honest lock
B1: `_mark_upload_failed_and_raise` now commits on a separate session
- Helper takes `request + upload_id`, opens a fresh session from
`request.app.state.session_factory` and commits there before raising.
- Closes the named-lock connection-pool leak Codex flagged: the old
"commit-on-the-same-session" implementation could return the
GET_LOCK connection to the pool before the enclosing
`finally: release_named_lock` ran, leaking `mp:<hash>` for up to
`pool_recycle` and re-opening the same-key upload race.
- Same helper now used by `create_server_object_payload`'s put-failure
branch — two failure paths have identical semantics.
B2: streaming copy for soft-delete + restore (`get_stream() + put()`)
- LOCAL backend: zero-copy (aiofiles stream write). OOM fixed.
- S3 backend: still OOMs on multi-GB objects — `put()` materializes
the async iter via `b"".join(chunks)`. Multipart `put` is a
follow-up; do NOT claim "OOM fixed on production" since production
defaults to S3.
C1: worker re-verifies `Users.status='active' AND is_deleted=0`
- `_assert_user_active` called from `_execution_context` after
resolving `triggered_by`; skips `SYSTEM_CRON_USER_ID`.
- `USER_DISABLED` error_code goes into the `NODE_FINISHED_EVENT`
outbox payload — `schedule_node_runs` has no `error_code` column,
the row only carries the `message` text. Docstrings corrected to
say so explicitly (previous docstring falsely promised row-level
observability).
F1: honest browser-local file lock
- `api.ts` `acquireFileLock/heartbeatFileLock/releaseFileLock/
releaseFileLockOnUnload` are now no-ops with comments stating they
never call the network.
- `scriptWorkspaceStore` dropped `tickHeartbeats`; `tickCleanup`
simplified to just clear cache.
- `useEditSessionLifecycle` dropped its 15s heartbeat `setInterval`.
- `ScriptWorkspace.tsx` renders `.local-lock-banner` info bar when
`isEditing`. Two tabs may still silently last-write — banner is the
only guard (acceptable disclosure-only tradeoff).
Dead code: deleted the duplicate `upload_bytes_to_session` in
`backend/src/backend/storage_api.py`. The `services.storage` import
is now the only source of the function; `create_upload_record`'s
docstring updated to point at `backend.resources`.
This commit is contained in:
@@ -27,6 +27,7 @@ from common.db.models import (
|
||||
ScheduleRuns,
|
||||
Schedules,
|
||||
StorageObjects,
|
||||
Users,
|
||||
Versions,
|
||||
Workspaces,
|
||||
)
|
||||
@@ -36,6 +37,7 @@ from common.eventing import (
|
||||
schedule_event_type,
|
||||
utcnow,
|
||||
)
|
||||
from common.scheduler.trigger import SYSTEM_CRON_USER_ID
|
||||
from loguru import logger
|
||||
from sqlalchemy import select
|
||||
|
||||
@@ -48,6 +50,13 @@ NODE_FINISHED_EVENT = schedule_event_type("job.node.finished")
|
||||
class NodeExecutor:
|
||||
"""Owns the actual execution of one schedule node (notebook / python)."""
|
||||
|
||||
# P0-5 / C1: distinct error_code for runs blocked because the originating
|
||||
# user was disabled or soft-deleted between queue time and worker pickup.
|
||||
# The value lands in the NODE_FINISHED_EVENT outbox payload (the
|
||||
# ``error_code`` field) — schedule_node_runs has no such column; the row
|
||||
# only carries the message text. Operators grep the outbox stream.
|
||||
USER_DISABLED_ERROR_CODE = "USER_DISABLED"
|
||||
|
||||
def __init__(
|
||||
self,
|
||||
*,
|
||||
@@ -123,18 +132,26 @@ class NodeExecutor:
|
||||
)
|
||||
except Exception as exc:
|
||||
trace = traceback.format_exc()
|
||||
# P0-5 / C1: 让 _assert_user_active 抛的 ValueError 透传成单独的
|
||||
# error_code,便于运维 grep 区分"用户被禁用"和"代码崩溃"。
|
||||
exc_message = str(exc)
|
||||
error_code = (
|
||||
self.USER_DISABLED_ERROR_CODE
|
||||
if exc_message.startswith("USER_DISABLED:")
|
||||
else "WORKER_EXECUTION_FAILED"
|
||||
)
|
||||
result = ExecutionResult(
|
||||
status="failed",
|
||||
exit_code=1,
|
||||
logs=trace.encode("utf-8", errors="replace"),
|
||||
result=json.dumps(
|
||||
{"status": "failed", "error": str(exc)},
|
||||
{"status": "failed", "error": exc_message},
|
||||
ensure_ascii=False,
|
||||
).encode("utf-8"),
|
||||
result_file_name=f"{payload['node_run_id']}-result.json",
|
||||
result_content_type="application/json",
|
||||
error_code="WORKER_EXECUTION_FAILED",
|
||||
error_message=str(exc)[:2000],
|
||||
error_code=error_code,
|
||||
error_message=exc_message[:2000],
|
||||
)
|
||||
if context is None:
|
||||
context = await self._fallback_execution_context(payload)
|
||||
@@ -299,6 +316,37 @@ class NodeExecutor:
|
||||
return "3.12"
|
||||
return row[0]
|
||||
|
||||
async def _assert_user_active(
|
||||
self,
|
||||
session: AsyncSession,
|
||||
user_id: str,
|
||||
) -> None:
|
||||
"""P0-5 / C1: re-verify the user is still ``status='active'`` and
|
||||
``is_deleted=0`` before executing a run they originated.
|
||||
|
||||
Raises :class:`ValueError` whose message starts with
|
||||
``USER_DISABLED:`` when the user has been disabled or soft-deleted
|
||||
between run creation and worker pickup. The outer
|
||||
:meth:`handle_node_execute` parses that prefix and routes the
|
||||
resulting ``error_code="USER_DISABLED"`` into the
|
||||
``NODE_FINISHED_EVENT`` outbox payload (the
|
||||
``schedule_node_runs`` row has no ``error_code`` column, only a
|
||||
``message`` text field).
|
||||
"""
|
||||
user = await session.scalar(
|
||||
select(Users.status, Users.is_deleted).where(Users.user_id == user_id)
|
||||
)
|
||||
if user is None:
|
||||
raise ValueError(
|
||||
f"USER_DISABLED: originating user {user_id} no longer exists"
|
||||
)
|
||||
status_value, is_deleted = user
|
||||
if status_value != "active" or is_deleted != 0:
|
||||
raise ValueError(
|
||||
f"USER_DISABLED: originating user {user_id} is "
|
||||
f"status={status_value!r} is_deleted={is_deleted}"
|
||||
)
|
||||
|
||||
async def _execution_context(
|
||||
self,
|
||||
payload: dict[str, Any],
|
||||
@@ -352,6 +400,13 @@ class NodeExecutor:
|
||||
if not storage.bucket_name or not storage.object_key:
|
||||
raise ValueError("stable version artifact location is incomplete")
|
||||
user_id = run.triggered_by or schedule.created_by
|
||||
# P0-5 / C1: re-verify the user is still active. ``create_scheduled_run``
|
||||
# checked membership when the run was queued, but the user may
|
||||
# have been disabled or soft-deleted in the meantime (admin
|
||||
# action, offboarding). Skip the check for the synthetic SYSTEM_CRON
|
||||
# user — that row is a fixed admin baseline and never goes inactive.
|
||||
if user_id != SYSTEM_CRON_USER_ID:
|
||||
await self._assert_user_active(session, user_id)
|
||||
context = {
|
||||
"node_status": node_run.node_status,
|
||||
"workspace_id": run.workspace_id,
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
"""Tests for NodeExecutor bucket-aware artifact download (P0-3).
|
||||
"""Tests for NodeExecutor bucket-aware artifact download (P0-3) and
|
||||
user-status re-verification (P0-5 / C1).
|
||||
|
||||
Pre-fix the worker's ``object_store`` was bound to the global version
|
||||
bucket at startup, so a workspace whose ``Workspaces.artifact_bucket``
|
||||
@@ -6,6 +7,11 @@ points at a custom S3 bucket would always 404 on download. The fix
|
||||
introduces a per-bucket store cache so the worker reads from whatever
|
||||
bucket the artifact actually lives in.
|
||||
|
||||
P0-5 / C1: the worker previously trusted the ``triggered_by`` user_id
|
||||
without re-checking ``Users.status`` / ``is_deleted``. A user disabled
|
||||
after a run was created would still have their schedules execute.
|
||||
The fix adds ``_assert_user_active`` to the execution-context path.
|
||||
|
||||
These tests cover only the routing logic — no live S3 / MySQL.
|
||||
"""
|
||||
|
||||
@@ -152,3 +158,46 @@ async def test_download_artifact_rejects_hash_mismatch() -> None:
|
||||
# Reads still went through the bucket-bound store, not the default.
|
||||
custom_store.get.assert_awaited_once()
|
||||
default_store.get.assert_not_called()
|
||||
|
||||
|
||||
# ── P0-5 / C1: user.status re-verification ────────────────────────────────
|
||||
|
||||
|
||||
async def test_assert_user_active_passes_for_active_user() -> None:
|
||||
"""The active path must not raise — the run is allowed to execute."""
|
||||
executor, _, _ = _make_executor()
|
||||
session = MagicMock()
|
||||
session.scalar = AsyncMock(return_value=("active", 0))
|
||||
|
||||
await executor._assert_user_active(session, "01USR0000000000000000000A")
|
||||
|
||||
|
||||
async def test_assert_user_active_blocks_inactive_user() -> None:
|
||||
"""A user whose status flipped to ``disabled`` after the run was
|
||||
queued must be rejected with a USER_DISABLED error_code-shaped prefix."""
|
||||
executor, _, _ = _make_executor()
|
||||
session = MagicMock()
|
||||
session.scalar = AsyncMock(return_value=("disabled", 0))
|
||||
|
||||
with pytest.raises(ValueError, match="USER_DISABLED"):
|
||||
await executor._assert_user_active(session, "01USR0000000000000000000A")
|
||||
|
||||
|
||||
async def test_assert_user_active_blocks_soft_deleted_user() -> None:
|
||||
"""``is_deleted=1`` (soft delete) must also block execution."""
|
||||
executor, _, _ = _make_executor()
|
||||
session = MagicMock()
|
||||
session.scalar = AsyncMock(return_value=("active", 1))
|
||||
|
||||
with pytest.raises(ValueError, match="USER_DISABLED"):
|
||||
await executor._assert_user_active(session, "01USR0000000000000000000A")
|
||||
|
||||
|
||||
async def test_assert_user_active_blocks_missing_user() -> None:
|
||||
"""Hard-deleted user (row gone) — also blocked, same prefix."""
|
||||
executor, _, _ = _make_executor()
|
||||
session = MagicMock()
|
||||
session.scalar = AsyncMock(return_value=None)
|
||||
|
||||
with pytest.raises(ValueError, match="USER_DISABLED"):
|
||||
await executor._assert_user_active(session, "01USR0000000000000000000A")
|
||||
|
||||
Reference in New Issue
Block a user