The drop-sync refactor is now contractually enforced:
* create_storage({"mode": "sync"|"async"|<any>}) raises StorageConfigError
-- the regression we most want to catch is someone "restoring" the
sync shape by re-introducing mode= handling.
* create_storage() preserves caller's dict, returns an AsyncStorageBackend
subclass, and wraps constructor TypeError into a useful StorageConfigError.
* build_storage_config emits no mode key on either local or s3 branch.
* register_backend() refuses to silently overwrite a different class on
the same name (collision guard), and accepts same-class re-registration.
* registry surfaces the built-in local + s3 classes after import; the
conflict-test cleanup pattern avoids leaking global state across tests.
Adds [tool.pytest.ini_options] (asyncio_mode=auto, testpaths=tests) so
uv run --package common pytest common/tests works from the workspace root.
17 new tests, all green. Backend suite still 136 passed.
The async-only direction was already the only one used in production:
* create_storage never accepted mode=sync; build_storage_config always
emitted mode=async; zero callers referenced StorageBackend / SyncData
/ S3StorageBackend.sync / LocalStorageBackend.sync anywhere.
* Drop the parallel sync base class, the sync concrete classes in
backends/local.py and backends/s3.py, and the boto3 dependency.
* Drop example_usage.py (zero importers; demonstration code, not part
of the public surface).
* Rename LocalAsyncStorageBackend -> LocalStorageBackend,
S3AsyncStorageBackend -> S3StorageBackend to reflect the single
remaining class per type.
* Tighten create_storage: any mode=... key now raises StorageConfigError
with the new pointer (settings.storage_backend controls behavior).
* Cleanup call sites: schedule.application.service.build_object_store
no longer passes mode=async to create_storage.
* Cosmetic touch-ups in backend/services/storage.py and
common/config.py docstrings where they still said "boto3" instead of
"S3 client".
Public API surface preserved: AsyncStorageBackend / ObjectMeta /
create_storage / build_storage_config / register_backend all keep
their names and call signatures. backend tests: 136 passed.
Review (2026-08-21) found that stage 1 promoted ExecutionResult from a
plain class to @dataclass(frozen=True) along the way. No caller mutates
or compares these objects by identity, so the only externally visible
change is structured log output. User opted to keep the upgrade.
- domain/execution.py module docstring: explicit note that the frozen +
value-equality form is a deliberate enhancement, not a behavioral
accident
- CLAUDE.md "Schedule service layering" lesson: add a "don't silently
upgrade dataclass-ness during a structural-only refactor" note so
future refactors copy class definitions verbatim unless they intend
to tighten semantics explicitly
No code change; tests still 29 green.
Co-Authored-By: Claude <noreply@anthropic.com>
Follow-up to the layered refactor (review-driven):
- Move scheduling/orchestrator.py -> application/orchestrator.py
(orchestrator is application-level coordination, not a cron-trigger
primitive; matches the intended target tree)
- Migrate orchestrator re-exports from scheduling/__init__.py to
application/__init__.py; scheduling/ now exposes only CronScheduler
- Rewrite imports + 5 mock.patch string targets in test_janitor.py and
the orchestrator import in test_layering.py
- Update docstring refs in application/service.py + execution/worker.py
- Add 4 runner smoke tests (test_layering.py): _limited_log under-limit /
empty-sentinel / above-MAX_LOG_BYTES truncation; execute_artifact
rejects unsupported script_type with ValueError
- infrastructure/__init__.py re-exports SchedulerStorageClient so
``from schedule.infrastructure import SchedulerStorageClient`` is a
stable top-level surface
- CLAUDE.md engineering note: extend the commit trail to 7476c27 and
note the stage-8 orchestrator placement
Zero behavior change; schedule/pyproject.toml untouched. 29 tests green.
Co-Authored-By: Claude <noreply@anthropic.com>
Pin the new five-layer contract so a later refactor can't silently break
an import surface or lifecycle:
- domain: ExecutionResult defaults, terminal/failed state-set invariants,
naive_utc normalization (import-side-effect-free)
- infrastructure.storage: SchedulerStorageClient base64 upload via
httpx.MockTransport (no live server; base_url required for relative URL)
- scheduling: CronScheduler start/close lifecycle, global trigger cleared
- application: SchedulerService wires cron + orchestrator + worker, handler
dispatch table points at the wired NodeExecutor
- execution: schedule.notebook_runner shim re-exports the real main
25 schedule tests green; schedule/pyproject.toml untouched.
Co-Authored-By: Claude <noreply@anthropic.com>
- Move service.py -> application/service.py (SchedulerService class name
unchanged; build_object_store / build_storage_http_client move along)
- main.py imports schedule.application.service
- Rewrite worker.py's lazy `from schedule.service import build_object_store`
and test_worker.py's mock patch string targets — same class of bug as the
test_janitor patch strings (silent no-op until the old file is deleted)
- Delete flat service.py (orphaned; only docstring refs remain in
orchestrator, cleaned up in stage 6)
- Zero behavior change; schedule/pyproject.toml untouched
Co-Authored-By: Claude <noreply@anthropic.com>
- Move worker.py -> execution/worker.py, executor.py -> execution/executor.py
(byte-identical copies; import sites updated)
- Merge old execution.py + notebook_runner.py into
execution/runners/notebook.py: subprocess CLI (main/emit_outputs) plus the
in-process helpers (_execute_notebook/_execute_python/execute_artifact)
- schedule/notebook_runner.py becomes a compatibility shim so
`python -m schedule.notebook_runner` (the worker's stable -m string) still works
- Delete flat execution.py (shadowed by the new execution/ package)
- Zero behavior change; schedule/pyproject.toml untouched
Co-Authored-By: Claude <noreply@anthropic.com>
Stage 3 of the layered refactor. Relocate the two scheduling components
into their own package so that domain / application / scheduling /
execution / infrastructure boundaries actually exist on disk.
- Add schedule/src/schedule/scheduling/__init__.py
- Move scheduler.py (232 lines) -> scheduling/scheduler.py
(byte-identical via diff; CronScheduler class name unchanged)
- Move orchestrator.py (946 lines) -> scheduling/orchestrator.py
(byte-identical via diff; DispatchOrchestrator + event constants
unchanged; NOT further split this round, per plan)
- service.py lines 39-40: import paths rewritten to the new module
- tests/test_janitor.py: rewrite the import + 5 patch() string targets
The 5 patch() targets ("schedule.orchestrator.session_scope" x3,
"schedule.orchestrator.asyncio.sleep" x2) were NOT caught by the
import-line grep — they patch module attributes at runtime and would
have become dead no-ops after the move (and would hard-raise once
the old module is deleted in stage 6). Rewriting them to
"schedule.scheduling.orchestrator.*" keeps the janitor tests meaningfully
exercising the new module.
- old flat scheduler.py / orchestrator.py left on disk; stage 6 deletes
them once all layers are extracted.
Validation:
- uv run --package schedule pytest schedule/tests -q: 18 passed
- uv run python -m compileall schedule/src: zero errors
- grep 'from schedule.(scheduler|orchestrator)\\b' (old paths): 0 matches
- grep '"schedule.orchestrator.' (old patch targets): 0 matches
- main.py / worker.py / domain/ / infrastructure/ / pyproject.toml
byte-identical to HEAD
Co-Authored-By: Claude <noreply@anthropic.com>
Stage 2 of the layered refactor. Move the storage HTTP client one
package deeper so that infrastructure code lives under a dedicated
namespace.
- Add schedule/src/schedule/infrastructure/__init__.py
- Add schedule/src/schedule/infrastructure/storage/__init__.py
- Add schedule/src/schedule/infrastructure/storage/client.py
(verbatim copy of old schedule/src/schedule/storage_client.py,
byte-identical via diff — 2682 bytes)
- main.py line 17: import path rewrite to the new module
(only consumer — service.py and worker.py take storage_client as
an `Any` constructor param and never imported the class)
- old schedule/src/schedule/storage_client.py left on disk; stage 6
deletes it once all layers are extracted.
Validation:
- uv run --package schedule pytest schedule/tests -q: 18 passed
- uv run python -m compileall schedule/src: zero errors
- grep 'from schedule.storage_client' (old path): 0 matches
- service.py and worker.py byte-identical to HEAD
- main.py / pyproject.toml / tests/ unchanged apart from the 1 import line
Co-Authored-By: Claude <noreply@anthropic.com>
Add the three files that complete stage 1 of the layered refactor:
- schedule/src/schedule/domain/__init__.py (empty package marker)
- schedule/src/schedule/domain/context.py
(TERMINAL_NODE_STATES / FAILED_NODE_STATES / TERMINAL_RUN_STATES / naive_utc —
pure types, no I/O)
- schedule/src/schedule/domain/execution.py
(ExecutionResult dataclass, frozen=True)
The corresponding import-path rewrites in worker.py / orchestrator.py /
scheduler.py / execution.py were already landed in cdcfcb2 (the prior
commit on this branch). This commit only adds the missing domain/
package files those imports point at.
Validation:
- uv run --package schedule pytest schedule/tests -q: 18 passed
- uv run python -m compileall schedule/src: zero errors
- from schedule.domain.context / schedule.domain.execution importable
- main.py / pyproject.toml / tests/ unchanged
Co-Authored-By: Claude <noreply@anthropic.com>
- Backend: GET /api/v1/data-resources accepts parent_path; LIKE
'{ws_id}/%/{escaped}/%' AND NOT LIKE '{ws_id}/%/{escaped}/%/%' on
StorageObjects.object_key (workspace-wide, escapes _ and %, mirrors
list_scripts parent_path semantics). 13 new tests in
test_resources.py (helper unit / SQL compile / SQLite behavioral).
- Frontend: listResources gains parentPath arg, propagated through
WorkspaceBoundApi + AuthContext binding. WorkspaceTreeGroup title
count and ScriptExplorer header count now include dataResources.
memberScriptGroups backfills data-only owners so users with only
data resources still render a group. loadDataResources accepts an
optional parentPath, default empty preserves prior behavior.
Codex review of ffec234 flagged:
1. **MEDIUM — loadScriptCount in-flight race on workspace switch.**
Previous implementation used `if (get().scriptCountLoading) return`
to dedupe. That meant switching workspaces WHILE a fetch was in
flight dropped the new fetch entirely; the stale response from the
previous workspace then overwrote state, leaving the dashboard
showing workspace A's total while the user is on workspace B.
Fix: drop the dedupe-via-flag, use a module-level
`_scriptCountSeq` counter. Every call increments, captures the
seq at start, and the response/finally block only mutates state
when `_scriptCountSeq === seq` — stale responses are silently
dropped. Rapid workspace switches each get their own fetch; only
the latest response wins.
2. **LOW — scriptCount scope comment was misleading.**
"范围与 list_scripts(parent_path=\"\") 对齐" is wrong: the
count includes all descendant depths, not just root-level.
Behaviour is correct for the dashboard's "全部脚本" intent but
the comment would mislead the next maintainer. Rewritten to
explicitly call out that the count is the UNION across all
parent_path depths, with the rationale for each design choice.
3. **LOW — local `from backend.scripts import _escape_like_pattern`
inside `resources.py` keyword-search block.**
The "avoid cycle" justification was false: scripts.py and
resources.py don't import each other at module level. Moved to
the top-of-file import block.
Skipped:
- get_workspace_tree `like_prefix` not run through the escape helper
(user_id is a 26-char Crockford ULID so no `_`/`%` can appear, but
the invariant is not documented at the call site). Pre-existing
pattern; out of scope for this round.
Verified: pytest 65 passed; pnpm typecheck clean.
Codex review of #36 + #37 surfaced that my prior `escape="\\"` only
declared the escape character — the pattern literals themselves still
contained unescaped `_` and `%`, so `parent_path="foo_bar"` continued
to match `fooXbar/...`, `foo2bar/...`, etc. My earlier ESCAPE-clause
assertions were tautological: they verified the SQL rendered the
ESCAPE keyword without ever checking that the pattern was actually
escaped. The tests passed; the leak persisted.
Fix in three layers:
1. Real escape: `backend/src/backend/scripts.py` gains
`_escape_like_pattern(value)` that escapes `\` → `\\`, `%` → `\%`,
`_` → `\_` (in that order — the escape char MUST be escaped first).
`_build_list_scripts_descendant_prefix` now returns the escaped
prefix. `list_workspace_directories` and `delete_workspace_directory`
also escape their server-built prefixes. `count_scripts` escapes
the user subtree prefix.
2. Same bug elsewhere: `backend/src/backend/resources.py:404` had the
identical `DataResources.resource_name.like(f"%{keyword}%")`
pattern; a search for "100%" would match everything. Now escaped
too.
3. Count endpoint scope: `count_scripts` was workspace-wide and
skipped the StorageObjects JOIN. Now INNER JOINs StorageObjects
(drops orphans whose current_object_id is dangling) and filters
by `workspace/{user_id}/` subtree so the result matches what
`list_scripts(parent_path="")` would return. Multi-member
workspaces no longer over-report, and orphan rows no longer
inflate the count.
Frontend: `DashboardRoute` is not keyed by workspace/user (only
ScriptsPage is), so without a workspace_id dep the previous
workspace's count persisted across navigation. useEffect now depends
on `currentWorkspace?.workspace_id`; `loadScriptCount` clears the
count to null at the start of the fetch so the dashboard doesn't
flash a stale number.
Tests — backend/tests/test_list_scripts_parent_path.py
- Rewritten with three layers of coverage:
* Pure helper tests for `_escape_like_pattern` (7 cases including
backslash-escape-first ordering).
* SQL-contract tests asserting the COMPILED PATTERN contains the
escaped form (lowercased to neutralise SQLAlchemy keyword casing).
* BEHAVIORAL tests on SQLite in-memory with the same LIKE
semantics — proves the fix actually prevents the wildcard leak.
Includes a negative test (without escape, siblings DO match) so
the fixture is verified to exercise the bug.
Tests — backend/tests/test_count_scripts.py
- Updated to assert the JOIN + user-scope filter. New test verifies
two different users in the same workspace get different subtrees.
Verified:
- pytest backend/tests: 65 passed (43 baseline + 12 list_scripts + 4 count + 6 helper/SQLite behavioral)
- pnpm typecheck: clean
- Raw SQL on MySQL (live DB) confirms `LIKE 'workspace/.../foo\_bar/%%' ESCAPE '\\'`.
After #34 the workspace store only holds the root-level scripts plus
whatever subfolders the user has expanded. DashboardRoute's
"全部脚本"/"工作副本" counts derived from scripts.length therefore
underreport the workspace total until the user navigates to /scripts
and expands every folder.
Fix: separate count endpoint + dedicated store field, mounted
independently.
Backend — backend/src/backend/scripts.py
- New endpoint GET /api/v1/scripts/count.
- Route declared BEFORE /api/v1/scripts/{script_id}/... so FastAPI's
declaration-order matching does not interpret "count" as a script_id.
- Returns { data: { total: number }, meta: {} }; SQL is a single
COUNT(*) on scripts filtered by workspace_id + status='active'.
Frontend — services/api.ts + context/AuthContext.tsx
- countScripts(workspaceId) client; WorkspaceBoundApi gains the field;
AuthContext binding forwards workspaceId.
Frontend — state/scriptWorkspaceStore.ts
- scriptCount: number | null, scriptCountLoading: boolean.
- loadScriptCount() action: idempotent (no-op while in-flight), silent
on failure (dashboard tolerates a stale count).
- Initial state and reset() clear both fields.
Frontend — features/platform/DashboardRoute.tsx
- Subscribes to scriptCount; calls loadScriptCount() on mount.
- Falls back to scripts.length until the count resolves so the
dashboard never blanks.
Tests — backend/tests/test_count_scripts.py (new)
- 3 unit tests: scalar result handling, NULL coercion, route callable.
Verified: pytest 55 passed (52 + 3 new); pnpm typecheck clean.
Codex review of #34 surfaced that folder names containing ``_`` (matches any
single character in SQL LIKE) or ``%`` (matches any sequence) would leak
across sibling paths when used as parent_path / descendant_prefix. Two
endpoints were vulnerable:
- list_workspace_directories (pre-existing, line 780/781, 814/815)
- list_scripts (added in #34, line 1101/1102)
Both had four LIKE calls total, all unescaped. ``safe_directory_name``
allows ``_`` and rejects nothing relevant, so any user-created directory
named e.g. ``foo_bar`` would have its queries silently match
``fooXbar/...`` paths too.
Fix: add ``escape="\\"`` to every .like() / ~.like() call so MySQL emits
``ESCAPE '\\'``. SQLAlchemy doubles the escape character for SQL string
literals (renders as ``ESCAPE '\\\\'``); test asserts the rendered form
appears for both positive and negated clauses in each endpoint.
Also touched get_workspace_tree's `like(like_prefix)` (line 707) and
delete_workspace_directory's `like(f"{child_prefix}%")` (line 1017) —
the prefixes are server-built so technically not user-controllable,
but defense in depth costs nothing.
Verified: pytest 52 passed (50 + 2 new ESCAPE assertions).
Codex (deepseek-v4-flash) review of feat(scripts) parent_path filter
surfaced four problems:
1. Critical — AuthContext listScripts binding silently dropped parentPath
- frontend/app/context/AuthContext.tsx:224 had:
listScripts: () => rawApi.listScripts(workspaceId)
so loadScripts("foo/bar") hit GET /api/v1/scripts with no query and
always received root-level scripts. Typecheck passed because
`() => ...` is assignable to `(parentPath?: string) => ...`.
- Fix: forward the parentPath argument.
2. High — load() cache invalidation gap on toolbar refresh / create / delete
- load() replaced `scripts` with root-only items but never invalidated
`loadedScriptPaths` for subfolders, so previously-expanded folders
rendered empty (cached no-op on re-expand) and open tabs pointing
into subfolders were dropped by the validIds filter.
- Fix: load() now re-fetches every path currently in
loadedScriptPaths (and loadedChildPaths for directories), then
dedups by id/path. On initial mount the cache is empty so this
degrades to a single root fetch.
3. Low — test docstring overclaimed coverage
- The "actual ORM roundtrip is covered by the existing integration
tests" line is false (no other list_scripts test exists).
- Fix: honest docstring noting the repo has no endpoint integration
test layer; SQL assertions are brittle to SQLAlchemy/dialect
formatting.
4. Low — backend docstring overclaimed index role
- Claimed `idx_storage_workspace_relative_path` "avoided全表扫", but
the index isn't declared in the ORM model, only exists via the
baseline migration's upgrade path, and EXPLAIN doesn't drive
through it (scripts-first plan via idx_scripts_workspace).
- Fix: accurate description — MySQL drives via idx_scripts_workspace,
storage_objects PK lookup applies LIKE per row. Notes where to
optimize if 10万-scale perf becomes a real problem.
Skipped findings (out of scope):
- Medium LIKE escape for `_` / `%` (pre-existing in
list_workspace_directories; not introduced here).
- Low dashboard `scripts.length` count underreport (separate UI bug,
pre-existing assumption that broke under the new semantics).
Verified: pytest 50 passed, pnpm typecheck clean.
Backend — backend/src/backend/scripts.py
- list_scripts accepts optional parent_path Query (default "").
- Extracts _build_list_scripts_descendant_prefix helper for the
"direct children of parent_path" prefix.
- WHERE clause now adds:
StorageObjects.relative_path LIKE '<prefix>/%'
AND NOT LIKE '<prefix>/%/%'
so deeper descendants and prefix-siblings (foo/bar vs foo/bar2)
are excluded. Empty parent_path filters to root-level only —
this is the symmetric, intent-aligned behavior the lazy-load
frontend relies on.
- EXPLAIN confirms idx_scripts_workspace drives the scripts table;
storage_objects PK lookup applies the LIKE filter per row.
Frontend — frontend/app/services/api.ts + scriptWorkspaceStore.ts
- listScripts accepts optional parentPath; built URL preserves
the new filter param.
- Store gains loadedScriptPaths / loadingScriptPaths Sets and a
loadScripts(parentPath) action: idempotent, in-flight dedupe,
dedup-by-id when merging into the flat scripts array so existing
find() callers keep working.
- load() now uses listScripts("") instead of bulk fetch — root
only on first paint.
- toggleExpanded() triggers loadScripts(parentPath) in parallel
with loadChildren(parentPath) so folder expansion loads both
sub-directories and direct-child scripts.
Tests — backend/tests/test_list_scripts_parent_path.py
- 7 unit tests: prefix construction, normalization, traversal
rejection, and SQL compilation contract (LIKE prefix + NOT LIKE
prefix + scripts.status filter).
Verified:
- pytest backend/tests: 50 passed (43 existing + 7 new)
- pnpm typecheck: clean
- alembic: no schema changes (migration-less feature)
- EXPLAIN with real workspace: idx_scripts_workspace → PK lookup
parent_object_id was never written or read by any application code (verified
via repo-wide grep: only the baseline migration and the ORM model referenced
it). The orphan index idx_storage_parent likewise served nothing.
Tree structure is maintained entirely via the materialized path in
storage_objects.relative_path (LIKE-prefix queries in
backend/src/backend/scripts.py: list_workspace_tree, list_workspace_directories).
The column mislead a prior review into proposing an adjacency-list table —
removing it eliminates that temptation for the next reader.
Migration: migrations/versions/f7a8b9c0d1e2_drop_storage_parent.py
- Drops idx_storage_parent first, then parent_object_id (correct order on MySQL).
- MySQL 8.0 has no IF EXISTS on DROP INDEX / DROP COLUMN, so calls are unconditional.
- Updates the storage_objects TABLE COMMENT so the materialized-path warning
reaches the DB, not just the ORM (Codex review finding).
- Downgrade restores both.
Model: common/src/common/db/models/storage.py
- Removes the dead Index entry and the dead column.
- Refreshes the table comment to flag the materialized-path contract.
Verified:
- alembic upgrade head: applied, head = f7a8b9c0d1e2
- SHOW INDEX / SHOW COLUMNS: 0 rows
- TABLE COMMENT updated in information_schema
- pytest backend/tests: 43 passed
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`.
S2: schedule worker add janitor task that force-terminals node_runs
whose deadline (timeout_seconds + retry_count*retry_interval + 120s
slack from started_at) has passed. Closes the gap where outbox retry
exhaustion (5 tries, capped 30s backoff) marked the *event* failed
but left the *node_run* stuck in queued/running forever. Re-reads the
row under FOR UPDATE before writing so a worker that races us to a
real terminal state is not overwritten; idempotency key uses
:timed_out variant so the :finished path cannot collide.
R1: extract _reap_once() from _reap_loop for testability; in the
dead-process branch, re-verify (process.pid, started_at) against the
live JUPYTER_PROCESSES entry before del. A start_workspace that
replaced the dead record mid-cycle used to have its new entry
silently erased by the reaper's stale snapshot — leaked the port.
R2: delete _drop_workspace_lock and its two call sites
(stop_workspace tail, get_workspace 404 path). Popping the lock
object after release breaks mutual exclusion for any coroutine still
holding the old reference while a fresh caller gets a new lock
object — same ws_id can race two starts. The dict is bounded by the
number of workspaces so the leak is negligible; invariant lives on
WORKSPACE_LOCKS in a comment.
Tests:
- schedule/tests/test_janitor.py — 8 tests covering normal kill /
healthy-skip / worker-race / never-started / multi-row batch /
cancellation propagation / per-iteration self-heal
- runtime/tests/test_process.py — 7 tests covering reaper identity
match / replacement-skip / alive-preserved + lock
same-object / concurrent-serialize / survives-stop /
helper-removed guard
uv run --package schedule pytest schedule/tests → 14 passed
uv run --package runtime pytest runtime/tests → 7 passed
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The fix lands in three concentric layers, all backed by a single
INTERNAL_SERVICE_TOKEN shared secret so we have one mechanism
instead of three:
1. docker-compose: drop the backend.ports: 8891:8000 and
runtime.ports: 8892:8000 mappings. Nginx is the only host
ingress again (architecture §2.2).
2. /internal/v1/*: the storage control plane had six endpoints, five
of which were dead code (frontend already migrated to
/api/v1/data-resources/* with JWT; schedule only ever called
POST /internal/v1/objects). Delete the dead routes, mount the
one survivor with Depends(require_internal_service) that
compares the X-Internal-Service-Token header against
settings.internal_service_token with secrets.compare_digest.
3. POST /api/v1/jupyter on the runtime container: previously open
inside the Docker network. Same token mechanism — backend's
runtime_http_client now carries the header, runtime's
handle_jupyter_action requires the same header. /api/v1/health
stays open for the Nginx and compose healthchecks.
The schedule worker was already configured to call
POST /internal/v1/objects; build_storage_http_client now
sets the token header so its existing call site keeps working
without changes.
Files touched:
backend/src/backend/storage_api.py # 5 dead routes deleted + token guard
backend/src/backend/main.py # runtime_http_client header
runtime/src/runtime/main.py # require_internal_service Depends
common/src/common/config.py # internal_service_token setting
schedule/src/schedule/service.py # httpx client header
docker-compose.yml # ports dropped, INTERNAL_SERVICE_TOKEN env
.env.example # INTERNAL_SERVICE_TOKEN placeholder
API.md / README.md / DEVELOP.md # §9 trimmed to 1 endpoint
Verified:
compileall -> 0 errors
pytest backend/tests -> 37 passed
in-process ASGI smoke:
POST /internal/v1/objects no/wrong/correct token -> 401/401/200
POST /api/v1/jupyter no/wrong/correct token -> 401/401/200
5 deleted internal routes -> 404
docker compose config (with env) -> OK
P0-1 still has one open sub-item (rclone RC --rc-no-auth) that
the user has explicitly deferred; not touched here.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary of changes:
- Resources Bind Idempotency (High 1+2):
- Check existing active storage object bindings before duplicate check.
- Return existing binding (`reused: true`) on retry with same upload_id.
- Filter by `status == "active"` to bypass dead/deleted rows during reuse check.
- Storage Concurrent Overwrite (High 3):
- Add `acquire_named_lock` and `release_named_lock` helpers using MySQL `GET_LOCK`/`RELEASE_LOCK` hashed to <= 64 chars.
- Wrap `upload_bytes_to_session` PUT+INSERT critical section with named lock on `object_key`.
- Re-check key collision inside lock; append ULID suffix on collision.
- Shared Reference Deletion Protection (Medium 4):
- Check active references before deleting storage objects in `delete_resource`.
- Delete only `DataResources` record if storage object is still referenced elsewhere.
- Robust Usage Type Fallback (Medium 5):
- Replace direct dict lookup for `USAGE_TYPE_TO_PURPOSE[item.usage_type]` with `.get(..., "workspace")` default.
- Idempotency Key Path Matching (Medium 6):
- Move `file_name`/`target_path` validation forward and include path dimension in comparison.
- Strip uniqueness suffix via `_strip_uniqueness_suffix` before key comparison to avoid false 409s on valid retries.
- Usage Type & Bind Concurrency Control (Low 7 & 8):
- Reject bind requests with 409 if upload session purpose is not `data_resource`.
- Wrap resource duplicate check and creation in named lock using `(owner, directory, name)`.
- Trash Key Uniqueness & Restore Compatibility (Low 9):
- Update `trash_key` format to `{purpose}/{object_key}-{storage_object_id}` to prevent collisions.
- Update `object_key_hash` on trash move.
- Update restore logic in `storage_api.py` to strip suffix while maintaining backward compatibility with legacy keys.
- Dead Code Removal (Low 10):
- Remove unreachable `upload_status = "failed"` and redundant `session.rollback()` in `IntegrityError` block.
- Tests & Mocks:
- Add/update 5 test cases covering non-data_resource bind rejection, suffix stripping, and path recovery.
- Add named lock statement mocks for DB testing.
tao.chenandtao.chen