From 69c34c9383818e6e7315816258f09ce5ef76d31e Mon Sep 17 00:00:00 2001 From: Emil Date: Wed, 22 Jul 2026 19:40:11 +0300 Subject: [PATCH] Simplify worker feedback in Russian --- docs/worker-vibecoder-feedback.md | 316 ++++++------------------------ 1 file changed, 57 insertions(+), 259 deletions(-) diff --git a/docs/worker-vibecoder-feedback.md b/docs/worker-vibecoder-feedback.md index dbbef6b..763ea89 100644 --- a/docs/worker-vibecoder-feedback.md +++ b/docs/worker-vibecoder-feedback.md @@ -1,259 +1,57 @@ -# Worker implementation: feedback for the next iteration - -## Purpose - -This is a technical debrief of the first `Workers` implementation. It is not a -blame document. Its purpose is to give an AI-assisted developer a compact set -of rules that prevents the same integration defects from returning when the Go -coordinator and PostgreSQL queue are implemented. - -The worker must be treated as one participant in a distributed protocol, not -as an isolated Python script. A change is only complete when the worker, -coordinator API, durable storage, documentation and tests agree on the same -contract. - -## What was good - -- The worker was separated into configuration, daemon, coordinator client, - artifact transport, runner and models. That is a sound seam for later Go - integration. -- Input checksum validation, task-attempt directories, lease heartbeats, - bounded failure messages, jittered polling, and mock-based tests were good - instincts. -- The daemon does not talk to PostgreSQL directly. Keeping queue ownership in - the coordinator is the right boundary. - -Keep these properties. The corrections below are about completing the protocol -rather than changing that general direction. - -## Findings from the review - -### 1. A worker-local result URI was presented as a completed result - -**Initial behaviour.** The worker submitted a `worker://...` URI after local -execution. - -**Why this is wrong.** The coordinator, reducer and UI cannot read a file that -exists only on a worker machine. A task cannot be considered complete merely -because a worker created a local CSV. The result disappears when that worker -is removed or its workspace is cleaned. - -**Correct rule.** Upload every output artifact to coordinator-managed durable -storage first. Only then submit a manifest containing the URI returned by the -upload endpoint, SHA-256 and content type. A result endpoint must reject a -`file://` or `worker://` URI. - -**Required order.** - -```text -run task locally - -> compute SHA-256 - -> PUT artifact to coordinator storage - -> coordinator returns durable URI - -> POST result manifest - -> coordinator marks task completed -``` - -**Regression test.** Test that a completion payload contains the URI returned -by `ArtifactClient.upload`, and that the worker performs no final `submit` if -an upload fails. - -Status: fixed in `Workers`; enforce the same rule server-side. - -### 2. Failures were sent to the success endpoint - -**Initial behaviour.** Exception handling reported a payload with a failed -status to `/tasks/{id}/result`. - -**Why this is wrong.** Completion and failure have different queue semantics. -A completion means the result is durable and can trigger reduction. A failure -may require a retry, backoff, attempt increment, or a terminal error. Combining -them makes it too easy for the coordinator to accept an invalid state -transition. - -**Correct rule.** Use `POST /tasks/{id}/result` only for a successful, -fully-uploaded result. Use `POST /tasks/{id}/failure` for a safe failure -report. Include `worker_id`, `attempt`, a stable `error_code`, and a short -sanitised `error_message`. - -**Regression test.** Make a runner raise; assert that `/failure` is called and -`/result` is never called. - -Status: fixed in `Workers`; the Go service must validate the transition. - -### 3. A heartbeat renewed the lease but the new expiry was discarded - -**Initial behaviour.** The client treated a successful heartbeat as a boolean -and retained the old `lease_expires_at` value. - -**Why this is wrong.** The worker used that stale timestamp to schedule future -heartbeats. Long-running work could then incorrectly decide that a valid lease -was expired, or choose an unsafe heartbeat interval. - -**Correct rule.** A successful heartbeat must return the new canonical -`lease_expires_at` timestamp. Replace the stored expiry immediately and derive -the next heartbeat delay from it. Treat a missing or malformed expiry as a -protocol error. - -**Regression test.** Return two different expiries from a fake coordinator; -verify that the second scheduling calculation uses the renewed value. - -Status: fixed in `Workers`; documented as a required coordinator response. - -### 4. The API contract was implemented in code before it was made explicit - -**Symptom.** The client and task documents initially omitted or disagreed on -artifact upload, failure reporting and the heartbeat response. - -**Why this is dangerous.** Two developers can write apparently reasonable -code that never interoperates. This is especially likely across Python and Go, -where type systems do not share the contract automatically. - -**Correct rule.** Before coding an endpoint, update the contract table and -provide request and response examples. State: - -- method and path; -- authentication and worker identity; -- mandatory JSON/body fields and types; -- success statuses and response body; -- idempotency behaviour; -- invalid ownership, stale attempt and expired-lease responses; -- whether the endpoint changes task state. - -The source of truth is [PLAN.md](../PLAN.md) plus the task-specific database -and worker documents. If a code change alters the protocol, update all of -them in the same commit. - -### 5. Client-side checks were treated as sufficient protection - -**Problem.** Python checks for input hash, task attempt and lease timing are -helpful but are not authoritative. A buggy, stale or malicious worker can -still send a request. - -**Correct rule.** The Go coordinator must make all authoritative decisions in -one database transaction: task is leased, `lease_owner` equals the caller, -attempt matches, lease is unexpired, and the requested state transition is -allowed. The worker is a client; it is never the queue authority. - -**Server tests.** Verify that another worker, a stale attempt and an expired -lease cannot upload artifacts, heartbeat, complete or fail the task. - -### 6. Happy-path testing hid cross-boundary defects - -**Problem.** Local mocks can make an endpoint mismatch invisible: the mock -accepts a payload that the actual Go coordinator has not implemented. - -**Correct rule.** Keep fast unit tests, but add contract and integration tests -as soon as the coordinator exists: - -- Python worker against a disposable Go coordinator and PostgreSQL database; -- claim → heartbeat → download → upload → result lifecycle; -- runner failure → failure endpoint → retry/terminal policy; -- duplicate delivery of result and failure payloads; -- coordinator restart with a leased task; -- unauthorized and cross-origin artifact cases. - -Every bug fixed at a boundary needs a regression test at that boundary, not -only an internal unit test. - -### 7. Transport security must be deliberate - -**What must not happen.** A coordinator bearer token must not follow an HTTP -redirect to arbitrary object storage or another host. - -**Correct rule.** Attach the coordinator bearer token only to same-origin -requests. Strip it on cross-origin redirects. Validate that coordinator URLs -are absolute `http` or `https` URLs. Never place a permanent token in task -payloads, logs or artifact URIs. - -Status: same-origin redirect protection and upload URL validation are now in -the Python worker. The coordinator still needs its own authentication and -authorization rules. - -### 8. Idempotency and retries need a written policy, not assumptions - -**Problem.** Network timeouts make it unknowable whether the coordinator -processed a request. Retrying can create duplicate artifacts or incompatible -state changes unless both sides define what is safe. - -**Correct rule.** The tuple `(task_id, attempt, worker_id)` identifies the -lease. Make completion, failure and artifact upload idempotent for that lease. -The coordinator should return a stable success response for an identical -duplicate and a conflict for a different worker or attempt. Do not quietly -overwrite an artifact belonging to a different attempt. - -**Do not do this.** Do not implement a retry loop that blindly repeats every -POST because it "usually works". Classify timeout/5xx, conflict, validation, -and authorization responses first. - -### 9. Readability matters at protocol boundaries - -**Observed risk.** Dense single-line payloads and broad exception blocks make -it hard to review fields such as `attempt`, artifact checksum or result URI. -These are correctness fields, not cosmetic details. - -**Correct rule.** Use named payload builders or multi-line dictionaries for -network messages. Keep methods short enough that the lifecycle is visible in -order: claim, lease, download, validate, run, upload, submit/fail, cleanup. -Use broad exception handling only at the daemon boundary, then report a -sanitised error. Do not silently swallow failures that should stop a task. - -## Mandatory pre-PR checklist for a worker change - -### Contract - -- [ ] The task/response JSON matches the documented API exactly. -- [ ] Worker ID, task ID and attempt are present where ownership is required. -- [ ] Heartbeat returns and the worker uses the renewed expiry. -- [ ] Result, failure and artifact endpoints have different, documented roles. -- [ ] Success and error status codes are handled intentionally. - -### Correctness - -- [ ] The runner computes only within its task-attempt directory. -- [ ] Input SHA-256 is checked before workload execution. -- [ ] Every output is uploaded before completion is submitted. -- [ ] Completion contains a durable URI, SHA-256 and content type. -- [ ] No result is submitted after a lost heartbeat/lease. -- [ ] The task's own identifier and attempt are never guessed or replaced. - -### Resilience and security - -- [ ] Transient coordinator failures use bounded backoff and do not spin. -- [ ] Failure reports have a bounded, sanitised message; no traceback, - workspace path, bearer token or secret is included. -- [ ] Authorization is sent only to the coordinator origin. -- [ ] Duplicate delivery and stale attempts have defined outcomes. -- [ ] Cleanup does not delete another task's or another attempt's files. - -### Validation - -- [ ] Unit tests cover successful run, checksum mismatch, runner failure, - upload failure, heartbeat failure and empty runner output. -- [ ] A test asserts no `worker://` or `file://` URI reaches completion. -- [ ] A test asserts that failure uses `/failure`, never `/result`. -- [ ] A test asserts that the renewed heartbeat expiry is used. -- [ ] Formatting, type checking and the full test suite pass. -- [ ] If the protocol changed, Go coordinator integration tests pass too. - -## How to approach the next task - -1. Read the relevant `CTX-*` section in [PLAN.md](../PLAN.md), plus the - worker and database task documents. -2. Write down the exact request/response and state transition before changing - Python or Go code. -3. Implement the smallest vertical slice across both boundaries. -4. Add a regression test for the unhappy path first discovered by review. -5. Review the diff specifically for data ownership, lease/attempt checks, - durable artifacts, retry behaviour and secrets in logs. -6. Only then expand functionality or refactor. - -## Short version for an AI coding prompt - -> SciMesh workers are untrusted clients of a coordinator-owned queue. Never -> complete a task until outputs are durably uploaded through the coordinator. -> Keep success and failure endpoints separate. Every lease-sensitive request -> carries worker ID and attempt, and the coordinator validates both -> transactionally. A heartbeat returns a new expiry that the worker must use. -> Treat retries as an idempotency design problem, not as a blind loop. Update -> code, API documentation and regression tests together. +# Памятка для доработки SciMesh Worker + +Это короткие правила по итогам ревью первой версии воркера. Перед новой +задачей прочитай также [PLAN.md](../PLAN.md) и документ задачи, который тебе +дали. + +## Что делать + +- Считай воркер клиентом. Очередью, статусами задач и PostgreSQL управляет + только coordinator. +- Сначала скачай и проверь входной файл по SHA-256, затем запусти расчёт. +- Сохраняй файлы только в своей папке `task_id/attempt`. +- После расчёта сначала загрузи результат через coordinator, затем отправляй + результат задачи. +- В `result` передавай URI, который вернул coordinator, плюс SHA-256 и тип + файла. +- При ошибке вызывай `/tasks/{id}/failure`; при успехе — + `/tasks/{id}/result`. +- Для heartbeat используй новое `lease_expires_at`, возвращённое coordinator. +- Во все запросы, связанные с задачей, передавай `worker_id` и `attempt`. +- Пиши тесты не только на успех: проверь ошибку runner, неверный checksum, + ошибку загрузки, потерю lease и пустой результат. +- Если поменял API, меняй одновременно Python-код, Go coordinator, + документацию и тесты. + +## Чего не делать + +- Не отправляй в coordinator пути вида `worker://...` или `file://...`. + Coordinator не видит локальные файлы воркера. +- Не используй `/result` для сообщения об ошибке. +- Не считай, что проверок Python достаточно: coordinator обязан сам проверить + владельца lease, attempt, срок lease и допустимость смены статуса. +- Не повторяй любой HTTP-запрос вслепую. Повторные запросы должны быть + безопасны: одинаковый `(task_id, attempt, worker_id)` не должен создавать + дубликаты. +- Не передавай bearer token на другой домен при редиректе и не пиши токены, + traceback или полные локальные пути в логи. +- Не помечай задачу завершённой, если артефакт не загружен надёжно. +- Не меняй контракт API «по ощущениям». Сначала зафиксируй JSON, статусы и + переходы состояний в документации. + +## Мини-чеклист перед коммитом + +- [ ] Результат загружен в storage coordinator до `POST /result`. +- [ ] Ошибка уходит в `POST /failure`, а не в `POST /result`. +- [ ] Heartbeat возвращает новый срок lease, и код его сохраняет. +- [ ] В запросах есть верные `worker_id` и `attempt`. +- [ ] Нет `worker://`, `file://`, токенов и секретов в результатах или логах. +- [ ] Добавлены тесты на новый сценарий и на ошибку. +- [ ] `pytest` и проверка форматирования проходят. + +## Короткий контекст для нейронки + +> SciMesh Worker — клиент coordinator, а не владелец очереди. Загружай +> результат через coordinator до завершения задачи. Успех отправляй в +> `/result`, ошибку — в `/failure`. Проверяй checksum, lease и attempt. +> Меняя протокол, обновляй Python, Go, документацию и тесты в одном изменении.