Repository navigation
fix: enforce lease limits and fence recommendation writes - #8
Conversation
- add a lease-fenced analysis queue with retries and dead letters - add JWT auth, tenant isolation, and server-side role checks - add Prometheus metrics, rate limiting, and an LLM circuit breaker - add the Alembic baseline, offline replay evaluation, and runbook - fence worker writes and close the four review findings Co-Authored-By: Claude Code <noreply@anthropic.com>
- Dead-letter expired analysis leases after max_attempts instead of requeueing forever; use conditional updates so a heartbeat is not overwritten by a stale recovery decision. - Verify worker lease ownership inside the recommendation write transaction and propagate LeaseLost so a stale worker aborts before writing. - Add consecutive lease-expiry/dead-letter and old/new worker interleaving tests; sync contract, runbook, and verification record. Co-Authored-By: Claude Code <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40a629ce67
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| The demo tenant keeps the raw key for backward compatibility with rows and | ||
| clients created before multi-tenancy. | ||
| """ | ||
| return key if tenant_id == "demo" else f"{tenant_id}:{key}" |
There was a problem hiding this comment.
When the demo tenant submits Idempotency-Key: acme:x, this helper produces exactly the same stored key as tenant acme submitting Idempotency-Key: x; colons are accepted by validate_key. Because both analysis and execution keys remain globally unique, the demo tenant can occupy another tenant's keys, causing otherwise valid analysis requests to return 409 and execution requests to fail their unique constraint. Use a composite (tenant_id, idempotency_key) constraint or an unambiguous namespace that also covers demo keys.
AGENTS.md reference: AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| The demo tenant keeps the raw key for backward compatibility with rows and | ||
| clients created before multi-tenancy. | ||
| """ | ||
| return key if tenant_id == "demo" else f"{tenant_id}:{key}" |
There was a problem hiding this comment.
On PostgreSQL, a non-demo tenant submitting a valid 100-character idempotency key now gets a database error: adding the tenant prefix exceeds the unchanged VARCHAR(100) columns on both Analysis.idempotency_key and Execution.idempotency_key. For example, tenant acme produces a 105-character stored value. SQLite does not enforce this length, so the SQLite tests miss the regression. Widen both columns with an existing-database migration or use a bounded encoding that preserves the accepted API key length.
Useful? React with 👍 / 👎.
| if row.status != "failed": | ||
| raise DomainError("analysis_not_retryable", "只有失败的分析可以手动重试", 409, {"status": row.status}) | ||
| previous_attempts = row.attempts | ||
| row.status = "queued" |
There was a problem hiding this comment.
If two retry requests read the same failed analysis before either commits, both pass the status check. After the first request commits and its background task claims or completes the job, the second request's unconditional ORM update can change that running or completed analysis back to queued. This invalidates the active run's status and permits another claim, potentially exceeding the allowed attempts. Perform the transition with a conditional UPDATE against the observed failed status and attempt, and reject the request when no row is changed.
Useful? React with 👍 / 👎.
| select(Analysis).where( | ||
| Analysis.status == "running", | ||
| or_(Analysis.lease_until.is_(None), Analysis.lease_until <= now), |
There was a problem hiding this comment.
If the process exits after run_analysis commits status="failed" for a retryable error but before process_claimed_job calls finalize_job, the analysis retains its worker ownership and lease without a scheduled retry. This recovery query only considers running rows, while claim queries only consider queued rows, so restarting and waiting for lease expiry never retries or dead-letters that job. Make failure persistence and retry scheduling atomic, or recover expired, unfinalized failed rows as well.
Useful? React with 👍 / 👎.
- enforce (tenant_id, idempotency_key) uniqueness for analyses and executions - widen stored keys to 200 characters and migrate legacy SQLite/PostgreSQL schemas - retry failed analyses with a single conditional UPDATE - recover failed jobs left between run_analysis and finalize_job - add SQLite/PostgreSQL regressions and update contract/runbook docs Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb02522720
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def upgrade() -> None: | ||
| bind = op.get_bind() | ||
| if bind.dialect.name == "sqlite": | ||
| _upgrade_sqlite() | ||
| else: | ||
| _upgrade_postgres(bind) |
There was a problem hiding this comment.
For a database created by the parent revision, the documented stamp 0001_baseline followed by upgrade head fails: those tables lack tenant_id and the analysis job columns, but stamping does not run the application's compatibility migrations. On SQLite, rebuilding from Analysis.__table__ attempts to copy columns that do not exist; PostgreSQL likewise cannot create the composite constraint without tenant_id. The legacy migration test misses this because its fixture already includes all those columns. Add an explicit schema upgrade before this revision, or ensure the documented migration entry point performs the prerequisite compatibility migration.
Useful? React with 👍 / 👎.
| route = request.scope.get("route") | ||
| record_http(request.method, getattr(route, "path", None) or path, response.status_code, duration) |
There was a problem hiding this comment.
With metrics enabled by default, unauthenticated requests to distinct nonexistent paths create permanently retained counter and histogram entries. This fallback passes the raw path to normalize_path, which only collapses UUIDs and numeric segments, so paths such as /probe-a, /probe-b, etc. remain distinct. Rate limiting does not bound this growth because the 429 branch also records each raw path. Sustained requests therefore grow API memory and the /metrics response without limit. Use a fixed unmatched-route label and bounded route labels in the pre-routing rejection branch.
Useful? React with 👍 / 👎.
…leases Migration (P1): build the baseline shape inside revision 0002 so a genuine pre-tenancy database, which can only be stamped 0001_baseline, upgrades in place instead of failing on missing tenant_id/job columns. SQLite disables foreign-key enforcement on the migration connection before any DML, because both table rebuilds drop a parent table while child rows still reference it. Tests now construct the real old schema (no tenant_id, attempts, lease_until) for both SQLite and PostgreSQL, assert every business row and FK survives, and cover re-running the upgrade plus the downgrade refusal. Metrics (P2): unmatched paths collapse to a fixed <unmatched> label, early 429 responses from rate limiting use <rate_limited>, and matched routes use the route template. Cardinality-bound tests cover random 404s and 429s. Leases (P3): create_recommendation, write_job, assert_owned, the initial claim checks, the crash handler, and LeaseHeartbeat now require the lease to still be live in SQL. A worker whose lease expired without a takeover can no longer create recommendations, persist results, or renew its own lease. finalize_job and requeue_expired keep their retry, dead-letter, and recovery semantics. Co-Authored-By: Claude Code <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd104c2219
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| def _prune(self, now: float) -> None: | ||
| if len(self._buckets) <= self._max_buckets: | ||
| return | ||
| stale = [key for key, bucket in self._buckets.items() if now - bucket.updated > 3600] | ||
| for key in stale: | ||
| self._buckets.pop(key, None) |
There was a problem hiding this comment.
An unauthenticated client can create unlimited buckets by rotating arbitrary Bearer values: request_context calls rate_limit_key before authentication, and each token hash becomes a new key. Once _max_buckets is exceeded, this pruning logic removes only entries older than one hour, so fresh entries continue accumulating and every new entry triggers a full scan while holding the lock on the request-processing thread. This permits both memory growth and progressively slower requests despite rate limiting being enabled. A local reproduction with max_buckets=8 retained all 10,000 newly created buckets. Enforce a hard capacity through bounded eviction or an overflow bucket rather than relying solely on age-based pruning.
Useful? React with 👍 / 👎.
- add isolated Playwright acceptance coverage for JWT/roles/tenancy, worker failure retry, and knowledge lifecycle - add E2E fixture and legacy duplicate migration review scripts - run browser E2E against a disposable database and forbid backend reuse - add Python 3.11 CI job mirroring the locked 3.12 checks - record actual acceptance results in TEST_REPORT and the sanitized review Co-Authored-By: Claude Code <noreply@anthropic.com>
目标
本轮只处理三项预审问题，不扩展功能：真实历史库迁移（P1）、指标标签基数（P2）、租约到期严格保护（P3）。基线
fb02522，当前 headdd104c2。实现
P1 真正的历史数据库迁移
backend/migrations/versions/0002_tenant_scoped_idempotency.py：升级开始时先在迁移连接上物化基线 schema（upgrade_legacy_schema），因此真实前租户库只需stamp 0001_baseline → upgrade head即可原地升级，不再因缺少tenant_id/ 任务字段失败。PRAGMA foreign_keys=OFF（pragma 在事务内会被忽略，且知识去重 UPDATE 会开启事务），两张表重建期间子表外键不会阻断父表 DROP；每行都复制到新表，关系保持不变，pragma 仅作用于该连接。backend/tests/legacy_schema.py按提交7fd0625的真实旧 schema 建库（无tenants/users、tenant_id、attempts/lease_until，保留uq_daily_scope与旧全局活跃文档索引），SQLite 与 PostgreSQL 都走官方 stamp → upgrade 路径，并断言 Analysis / Recommendation / Execution / Audit / Knowledge 行数、外键关系与业务唯一约束保留。P2 指标标签基数
route_label(scope)优先使用路由模板；未匹配路径统一<unmatched>；限流提前返回的 429 统一<rate_limited>，不再按原始 URL 生成标签。P3 租约到期严格保护
lease_is_live(now)在数据库侧比较lease_until（NULL 视为无租约；恰好等于now视为已到期，与requeue_expired的<= now一致）。create_recommendation、write_job、assert_owned、初始抢占检查、崩溃处理与LeaseHeartbeat都追加租约有效性条件，并使用synchronize_session=False避免 SQLite naive/aware datetime 在 Python 侧求值。finalize_job/requeue_expired未改，保留既有重试、死信与恢复语义。实际执行的验证
PROFITOPS_TEST_POSTGRES_URL=... .venv/Scripts/python.exe -m pytest backend/tests -q.venv/Scripts/python.exe -m pytest backend/tests/test_migrations.py -qstamp 0001_baseline → upgrade head；行数/外键/uq_daily_scope保留；列宽 200、复合唯一、租约字段、索引到位；重复 upgrade no-op；跨租户同键可用、同租户重复拒绝；重复活跃文档软删除；downgrade 冲突时失败且保留行PROFITOPS_TEST_POSTGRES_URL=... .venv/Scripts/python.exe -m pytest backend/tests/test_postgres_migrations.py -q.venv/Scripts/python.exe -m pytest backend/tests/test_observability.py -q<unmatched>；匹配路由 →/api/analyses/{analysis_id}；25 个随机 429 → 一个<rate_limited>.venv/Scripts/python.exe -m pytest backend/tests/test_worker.py -qwrite_job/create_recommendation）、assert_owned、心跳延迟、已被接管、活跃租约正常续租npm --prefix frontend test -- --runnpm --prefix frontend run buildLLM_PROVIDER=disabled：python scripts/smoke.py --base-url http://127.0.0.1:8015python scripts/migrate.py upgrade head/current/python -m alembic -c backend/alembic.ini check/downgrade base0002_tenant_scoped_idempotency (head)；No new upgrade operations detected；降级成功dd104c2）迁移测试与冒烟均使用独立临时数据库，未修改
data/profitops.db。未执行 / 未验证
40a629c）已在临时库完成真实taskkill /F与重启恢复；本轮 P3 只增加写入时的租约校验，恢复路径未改，由 22 个 worker 测试覆盖。LLM_API_KEY为空，自动 CI 同样为空）。.venv为 3.12.8，未单独验证。剩余风险
docs/RUNBOOK.md）。🤖 Generated with Claude Code