feat(api): implement production unit management (Order/Batch/Task/Episode)#23
feat(api): implement production unit management (Order/Batch/Task/Episode)#23shark0F0497 merged 3 commits intomainfrom
Conversation
… for orders, batches, tasks, and episodes
…c into a utility function
internal/api/handlers/task.go
Outdated
| c.JSON(http.StatusBadRequest, gin.H{"error_msg": "invalid task id"}) | ||
| return | ||
| } | ||
| if idStr == "" { |
There was a problem hiding this comment.
SUGGESTION: Dead code — idStr == "" is unreachable here
strconv.ParseInt("", 10, 64) always returns an error, so the if err != nil check on line 358 already catches empty strings and returns early. This block can never be reached and should be removed.
| if idStr == "" { | |
| } | |
| var req UpdateTaskRequest |
internal/api/handlers/transfer.go
Outdated
| FROM episodes | ||
| WHERE task_id = ? AND deleted_at IS NULL | ||
| LIMIT 1 | ||
| `, taskRow.ID).Scan(&existingEpisodeID); err != nil || existingEpisodeID == "" { |
There was a problem hiding this comment.
WARNING: Idempotency check treats all DB errors as "episode does not exist"
The condition err != nil || existingEpisodeID == "" proceeds to insert a new episode for any error — including real database failures (connection drops, timeouts), not just sql.ErrNoRows. This could lead to duplicate episode creation under transient DB errors. Consider checking specifically for sql.ErrNoRows:
| `, taskRow.ID).Scan(&existingEpisodeID); err != nil || existingEpisodeID == "" { | |
| `, taskRow.ID).Scan(&existingEpisodeID); err != nil && err != sql.ErrNoRows { | |
| logger.Printf("[TRANSFER] Device %s: DB query failed for task=%s: %v", dc.DeviceID, taskID, err) | |
| return | |
| } | |
| if err == sql.ErrNoRows || existingEpisodeID == "" { |
| status = 'completed', | ||
| completed_at = CASE WHEN completed_at IS NULL THEN ? ELSE completed_at END, | ||
| updated_at = ? | ||
| WHERE id = ? AND deleted_at IS NULL |
There was a problem hiding this comment.
WARNING: Task status unconditionally overwritten to completed
The WHERE id = ? AND deleted_at IS NULL clause does not guard on the current status. If a task was manually set to cancelled or failed, a retried upload callback would silently overwrite it back to completed. Consider adding a status guard:
| WHERE id = ? AND deleted_at IS NULL | |
| WHERE id = ? AND status NOT IN ('cancelled', 'failed') AND deleted_at IS NULL |
There was a problem hiding this comment.
State machine management will be implemented uniformly later.
Code Review Summary (Incremental — commit
|
| Severity | Count |
|---|---|
| CRITICAL | 0 |
| WARNING | 1 |
| SUGGESTION | 0 |
Incremental Changes
Two previous issues were fixed in this commit:
| File | Previous Issue | Status |
|---|---|---|
internal/api/handlers/task.go |
Dead code: idStr == "" unreachable after ParseInt |
✅ Fixed (moved before ParseInt) |
internal/api/handlers/transfer.go |
Idempotency check treats all DB errors as "episode does not exist" | ✅ Fixed (now uses errors.Is(err, sql.ErrNoRows)) |
Remaining Issues (click to expand)
WARNING
| File | Line | Issue |
|---|---|---|
internal/api/handlers/transfer.go |
449 | Task status unconditionally overwritten to completed — cancelled/failed tasks can be overridden by upload retries |
Files Reviewed (2 files changed in this increment)
internal/api/handlers/task.go- 0 new issues (1 previous issue fixed)internal/api/handlers/transfer.go- 0 new issues (1 previous issue fixed, 1 carried forward)
Pull Request Checklist
Please ensure your PR meets the following requirements:
Summary
This PR implements a complete production unit management system for Keystone Edge, adding Order, Batch, Task, and Episode CRUD operations with idempotency guarantees, transactional consistency, and referential integrity controls.
Motivation
The production workflow requires managing the full lifecycle of data collection tasks:
This change establishes the Edge-side closed-loop for task generation, recording, upload, and episode persistence, enabling downstream QA, sync, and search capabilities.
Changes
Modified Files
internal/api/handlers/task.go- Enhanced task creation with batch management, batch reuse logic,target_countenforcement, idempotent episode creation via Transfer ACK, and replacement of mock data with real queries inGetTaskConfiginternal/api/handlers/transfer.go- Added idempotency check for episode creation, batchepisode_countmaintenance, and automatic task status transition tocompletedafter upload verificationinternal/api/handlers/episode.go- FixedTaskIDfield to use numeric task PK directly instead of JOIN, and added backward compatibility for legacy public task_id string filteringinternal/api/handlers/scene.go- Added referential integrity check preventing scene deletion when referenced by ordersinternal/server/server.go- Registered newBatchHandlerandOrderHandlerroutes under/api/v1internal/storage/database/migrations/000001_initial_schema.up.sql- Madeorders.nameNOT NULL, simplified_name_uniquegenerated column to excludescene_idfor flexibilityAdded Files
docs/designs/production-units.md- Design document defining domain relationships, data model semantics, HTTP API contracts, state machines, and key flows for Order→Batch→Task→Episode lineageinternal/api/handlers/batch.go- BatchHandler with ListBatches, GetBatch, DeleteBatch (soft delete), PatchBatch (status transitions with state machine validation)internal/api/handlers/order.go- OrderHandler with ListOrders, GetOrder, CreateOrder, UpdateOrder, DeleteOrder (with referential integrity checks for batches/tasks/episodes)Type of Change
Impact Analysis
Breaking Changes
None
Backward Compatibility
GET /tasks/:idnow uses numeric task PK (tasks.id) instead of public task_id string (tasks.task_id). Existing clients using numeric IDs will continue to work.GET /episodesnow returnstask_idasint64instead ofstring. Clients expecting string type will need to update.(organization_id, scene_id, name)to(organization_id, name). Existing orders with duplicate names across scenes may be affected.Testing
Test Environment
Test Cases
Manual Testing Steps
POST /ordersPOST /tasks(verifies batch creation/reuse, target_count enforcement)GET /tasks/:idandGET /tasks/:id/configcompletedafter Transfer Verified ACKTest Coverage
Screenshots / Recordings
Performance Impact
Documentation
Related Issues
Additional Notes
Key design decisions:
POST /tasksreuses existing active/pending batches for the same(order_id, workstation_id)pair to avoid fragmenting batchestask_idbefore inserting, preventing duplicates on device retrybatches.episode_countis incremented atomically within the same transaction as episode creationcompletedby Transfer Verified ACK, not by device callbacksReviewers
@kilo-code-bot
Notes for Reviewers
FOR UPDATElockingtransfer.gohandles all retry scenariosChecklist for Reviewers