Skip to content

Serialize ordinary Video state-save request lifetimes - #309

Draft
lifeofgurpreet wants to merge 1 commit into
openedx:mainfrom
lifeofgurpreet:fix/video-save-state-order
Draft

lifeofgurpreet wants to merge 1 commit into
openedx:mainfrom
lifeofgurpreet:fix/video-save-state-order

Conversation

@lifeofgurpreet

@lifeofgurpreet lifeofgurpreet commented Oct 2, 2026 •

Copy link
Copy Markdown

Two ordinary Video state saves can be in flight together. If a newer position reaches save_user_state before an older request, the native handler accepts both and the older request can move saved progress backward.

Keep a per-instance queue of payload snapshots and dispatch the next ordinary save when the current jqXHR settles. Retain native storage updates, clock formatting, URL, request shape and synchronous/asynchronous modes. An unload-triggered call joins an existing queue instead of starting an additional parallel write. Update the shared save-user-state test fixture to return the native Deferred/promise protocol and register a focused Jasmine regression.

Fast diagnostic validation: five Node checks execute the actual plugin source and its actual clock formatter with bounded jQuery/DOM/HTTP protocols. The byte-identical parent plugin from f39c61d4f383af46914d7e0aa7b0d554489f1345 fails four ordering checks and passes the disabled/idle positive control. Captured actual request bodies replayed through the real installed xblocks-contrib0.16.1 Video handler and XBlock field saves preserve the newer position in serialized order; the parent's concurrently emitted bodies in reversed commit order accept both saves and finish at the older position. The native handler method is AST-identical to current source. Node syntax and diff checks pass; scoped unsuppressed UBS has zero critical issues and 52 warnings, including inherited helper/fixture code. The full Karma/browser/Jasmine suite and production bundle rebuild were not run.

This serializes client request lifetimes, not server transactions after an abort, timeout or other ambiguous transport failure. Those can still settle a jqXHR while its server operation is unresolved. Native retries remain unchanged; no compare-and-save, cross-tab, shared-database or durable transport guarantee is claimed. A destroyed context may never dispatch the queued latest state, so page-close resume delivery remains unimplemented/unaccepted. This draft is an ordinary-save ordering improvement for independent integrity and maintainer review; it does not accept the broader unload feature or a deployed LMS/image/tenant/browser.

The concrete active-request unload tradeoff is deliberate and still red for product acceptance: with an old position-10 XHR active, an unload call at position 90 previously attempted an additional synchronous position-90 request. This revision queues that attempt until the first jqXHR settles. If the page context dies without its callback, the last emitted position remains 10. The unchanged parent passes that dispatch-intent assertion by emitting 90, while this revision fails it; the parent's attempt can still be overwritten by the late old save. Neither arm proves real browser delivery or durable saved state. This contribution does not resolve that broader requirement and is not included in the current LMS source carrier.

Run node --test tests/test_video_save_order.js. For the identical negative contract set VIDEO_SAVE_STATE_SOURCE to the byte-identical parent plugin. Native replay receipts and captured bodies are retained under /data/tmp/verawood-epic/GPT-B/gbc7/upstream-video-20261002/.

Queue per-instance payload snapshots while a jqXHR is active. Preserve native request and cache semantics; ambiguous transport failure, destroyed-context delivery and durable server ordering remain unqualified.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Oct 2, 2026
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @lifeofgurpreet!

This repository is currently maintained by @openedx/axim-engineering.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.
🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads
🔘 Submit a signed contributor agreement (CLA)

⚠️ We ask all contributors to the Open edX project to submit a signed contributor agreement or indicate their institutional affiliation.
Please see the CONTRIBUTING file for more information.

If you've signed an agreement in the past, you may need to re-sign.
See The New Home of the Open edX Codebase for details.

Once you've signed the CLA, please allow 1 business day for it to be processed.
After this time, you can re-run the CLA check by adding a comment below that you have signed it.
If the CLA check continues to fail, you can tag the @openedx/cla-problems team in a comment for further assistance.

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.


Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@lifeofgurpreet

Copy link
Copy Markdown
Author

Independent C scoped integrity review: no additional blockers in the assigned ordinary client request lifetime increment, exact cdf1113e65699da5deb9d4fdc1205107e64c145e over f39c61d4f383af46914d7e0aa7b0d554489f1345. This is diagnostic source qualification only; the broader page-close/resume requirement remains RED and is not accepted by this verdict.

Read all four changed source/test/helper paths. Fresh execution of the actual plugin and clock formatter: 5 PASS; the identical contract against the byte-identical parent: 4 assertion FAIL /1 positive PASS. The per-instance snapshots/queue preserve native payload fields, cache updates, clock formatting and dispatch modes; .always releases the next save after success or error settlement. Observer calibration using the exact vendored jQuery2.2.4 Callbacks/Deferred component, with DOM/HTTP still bounded protocols, repeats 5 PASS vs4 FAIL/1 PASS, including immediately resolved synchronous promises. This is not full jQuery/browser/AJAX transport or Karma/Jasmine qualification.

Fresh captured outgoing bodies replayed through the real installed xblocks-contrib0.16.1 native Video handler plus XBlock DictFieldData save show serial10→73 retaining73, while parent concurrent bodies applied in reverse73→10 retain10. The native handler method is AST-identical to current source. This tests the real handler/field consumer; it is not deployed database durability or a server transaction guarantee. The shared Jasmine fixture now returns the Deferred/promise protocol; the ordinary-save regression is registered. Source/spec syntax and diff checks pass. Full Karma/browser/Jasmine and production bundle build were not run; B's0critical/52warning UBS qualification remains disclosed, with no broader scanner-clean claim.

Concrete page-close tradeoff: when10 is in flight and unload observes90, the parent immediately attempts the synchronous90 save; the candidate queues90 until the earlier jqXHR settles. With no surviving callback, the candidate's last emitted position remains10. C's broader original close-attempt control therefore FAILS on the candidate and PASSES dispatch intent on the parent. That parent pass is not durable delivery: its earlier request can still overwrite90 later. This is the disclosed unresolved closing-context requirement, not proof that this contribution preserves latest state on close. No server ordering after abort/timeout/ambiguous failure, cross-tab or destroyed-page acceptance is granted.

Replay: node --test tests/test_video_save_order.js; VIDEO_SAVE_STATE_SOURCE=<exact parent plugin> selects the same negative contract. Fresh C source hashes/component/control/native receipts: /data/tmp/gpt-lanes/GPT-C-upstream-video-review-20261002/review-identity.json; logs GPT-C-upstream-video-{client,parent,native,native-deferred,parent-native-deferred,page-close,parent-page-close}-20261002.log under /data/tmp/gpt-lanes/. No source edits, preview hold, image/package promotion or runtime data action. The owned C review worktree is removed and all C processes are terminal. Complete native browser/three-tenant Video behavior, intended LMS package/image consumption, and safe latest-state delivery remain OPEN on the existing LMS XBlock workstream.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

Status: Needs Triage

Development

Successfully merging this pull request may close these issues.

2 participants