feat: add launch and progress projections - #7
Conversation
|
Please perform an independent review of the exact current head |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head review request for 72cfe7b. Re-verify the exact-head diff, required Checks, tenant/security boundaries, and any valid review findings after the shared Rust component-install fix. COMMENT only; no approval or protected-merge bypass is requested.
# Conflicts: # .github/workflows/quality.yml # CHANGELOG.md # README.md # docs/ARCHITECTURE.md # docs/product-technical-gap-baseline.md
|
@OpenCode review only the exact current head 00269c3 against base 4cfb77f. Re-check attempt-to-registration tenant keys, out-of-order progress rejection/idempotency, 14-policy RLS coverage, rollback dependency order, and the reconciled parent workflow. Leave an independent review or approval for this exact head only. Do not update code, merge, self-approve, bypass protections, or treat queued Checks as proof. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| FROM learning_attempt \ | ||
| WHERE tenant_id = $1 AND learner_id = $2 AND learning_attempt_id = $3 \ | ||
| AND attempt_status IN ('launched', 'active') \ | ||
| ON CONFLICT (tenant_id, learning_attempt_id, source_authority, \ | ||
| external_activity_reference, source_version) DO UPDATE \ | ||
| SET source_digest = EXCLUDED.source_digest, \ | ||
| progress_state = EXCLUDED.progress_state, \ | ||
| progress_percent = EXCLUDED.progress_percent, \ | ||
| observed_at = EXCLUDED.observed_at, \ | ||
| recorded_at = now() \ | ||
| WHERE progress_projection.observed_at <= EXCLUDED.observed_at \ | ||
| RETURNING progress_projection_id", | ||
| ) | ||
| .bind(tenant_id) | ||
| .bind(learner_id) | ||
| .bind(learning_attempt_id) | ||
| .bind(&request.source_authority) | ||
| .bind(&request.external_activity_reference) | ||
| .bind(&request.source_version) | ||
| .bind(&request.source_digest) | ||
| .bind(&request.progress_state) | ||
| .bind(request.progress_percent) | ||
| .bind(observed_at) | ||
| .fetch_optional(&mut *transaction) | ||
| .await? | ||
| .ok_or(ApiError::BadRequest( | ||
| "active learning attempt or newer progress observation is required", | ||
| ))?; | ||
| let progress_projection_id: Uuid = projection.try_get("progress_projection_id")?; | ||
| let next_attempt_status = if request.progress_state == "completed" { | ||
| "completed" | ||
| } else { | ||
| "active" | ||
| }; |
There was a problem hiding this comment.
🟡 Duplicate or corrected completion updates from the learning record store are wrongly rejected
Once a learning attempt is marked finished (attempt_status = 'completed' set at crates/lms_kernel/src/bin/lms_api.rs:727-742), any further progress observation is filtered out (attempt_status IN ('launched', 'active') at crates/lms_kernel/src/bin/lms_api.rs:700) and returns an error, so a re-sent identical completion is treated as invalid.
Impact: A finished learner activity that receives a repeated or corrected completion signal returns an error instead of succeeding, breaking the promised idempotency and blocking progress for any other activity in that attempt.
Filter excludes completed attempts, defeating idempotency and multi-activity progress
The progress upsert only proceeds when the attempt row matches attempt_status IN ('launched', 'active') (crates/lms_kernel/src/bin/lms_api.rs:698-700). After a first observation whose progress_state is "completed", the follow-up UPDATE sets attempt_status = 'completed' (crates/lms_kernel/src/bin/lms_api.rs:727-742).
Consequences:
- Idempotency violation: an LRS delivers statements at-least-once. Re-delivering the exact same completion observation (equal
observed_at, same source key) now finds nolaunched/activeattempt, so theINSERT ... SELECTyields zero rows,fetch_optionalisNone, and the handler returnsApiError::BadRequest→ HTTP 400 (crates/lms_kernel/src/bin/lms_api.rs:721-725). The PR explicitly claims "repeated newer/equal observations idempotent". - Multi-activity lockout:
progress_projection_source_uniquekeys on(tenant_id, learning_attempt_id, source_authority, external_activity_reference, source_version)(migrations/0001_learning_kernel.sql:261-263), implying multiple activities per attempt. Completing one activity marks the whole attemptcompleted, so any other activity's progress POST returns 400. - Correction lockout: the
observed_at-based out-of-order guard (crates/lms_kernel/src/bin/lms_api.rs:708) can never accept a legitimately newer post-completion correction because the attempt is already excluded by status.
The CI smoke test only exercises a single in-progress observation followed by an older one, so it does not surface this path.
Prompt for agents
In record_progress (crates/lms_kernel/src/bin/lms_api.rs), the INSERT ... SELECT filters the target attempt with attempt_status IN ('launched', 'active'), but the subsequent UPDATE promotes the attempt to 'completed' whenever the observation's progress_state is 'completed'. As a result, once an attempt is completed, all further progress POSTs (including an idempotent re-delivery of the same completion observation, a newer correction, or progress for a different activity of the same attempt) find no matching launched/active attempt and return HTTP 400. This contradicts the stated idempotency guarantee (repeated newer/equal observations should be idempotent) and breaks multi-activity attempts. Consider allowing 'completed' attempts to still accept progress upserts (e.g. include 'completed' in the SELECT filter), relying on the observed_at-based ON CONFLICT WHERE guard for ordering, and reconsider whether a single activity's 'completed' state should mark the entire attempt completed when multiple activities per attempt are possible. Ensure the closed_at reset logic does not incorrectly re-open a genuinely completed attempt.
Was this helpful? React with 👍 or 👎 to provide feedback.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| --output "$RUNNER_TEMP/lms_registration_response.json" | ||
| python3 -c 'import json, sys; assert json.load(open(sys.argv[1]))["registration_status"] == "registered"' "$RUNNER_TEMP/lms_registration_response.json" | ||
| registration_id=$(python3 -c 'import json, sys; print(json.load(open(sys.argv[1]))["learning_registration_id"])' "$RUNNER_TEMP/lms_registration_response.json") | ||
| attempt_id=$(curl --fail --silent --show-error --request POST \ |
| --data '{"external_attempt_reference":"attempt-ci-1","content_release_reference":"content-release/v1/partner-basics"}' \ | ||
| "http://127.0.0.1:8080/v1/tenants/$tenant_id/learners/$learner_id/registrations/$registration_id/attempts" \ | ||
| | python3 -c 'import json, sys; print(json.load(sys.stdin)["learning_attempt_id"])') | ||
| curl --fail --silent --show-error --request POST \ |
2 participants
Summary
Evidence
00269c3fadba66596e696f544fcbd5b78540c54d4cfb77f5403cba5fd53c37a794da1d2695ecf999actionlint .github/workflows/quality.ymlcargo +1.97.1 fmt --all -- --checkcargo +1.97.1 test --workspace --all-targets --lockedcargo +1.97.1 clippy --workspace --all-targets --locked -- -D warningsRUSTDOCFLAGS='-D warnings' cargo +1.97.1 doc --workspace --no-deps --locked201, older progress observation400, persisted progress row1git diff --checkScope boundary
This is the launch/progress slice of issue #3. LRS client contracts, assessment, completion execution, credentials, browser E2E, observability, hot partitions, and production readiness remain follow-up work.
Review request
Please review the exact current head
00269c3fadba66596e696f544fcbd5b78540c54dagainst4cfb77f5403cba5fd53c37a794da1d2695ecf999. Do not treat local or green CI checks as semantic approval.