docs: state the real HTML5 save mechanism, from engine source (#20) #61
No reviewers
Labels
No labels
area:defold
area:docs
area:infra
area:protocol
area:testing
kind:bug
kind:chore
phase:breadth
phase:core
phase:hardening
phase:slice
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
yuris/spacetimedb-defold!61
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "docs/20-html5-durability"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #20. Phase 2 of the v1.1.0 milestone (#59). Documentation only — no SDK
code change, and that is the finding rather than a shortfall.
The question, answered from engine source
#20's first checkbox asks whether Defold's HTML5
sys.savecallsFS.syncfsoronly writes the MEMFS layer, and requires the answer come from engine source
rather than observed behaviour. Read at
defold/defoldtag1.13.1=574678c7d44be490d874fbed2d0ae6211feec4d9, with the loader patch additionallyconfirmed present at tags 1.4.8, 1.6.0, 1.9.5, 1.10.4, 1.11.2, 1.12.4 and branch
dev— i.e. every Defold a consumer could plausibly be running.Verdict: it syncs — but not inside
sys.save, and not synchronously.Sys_Save's__EMSCRIPTEN__branch,engine/script/src/script_sys.cpp:202-218,is
fopen(filename, "wb")→fwrite→fclose. No sync. The engine C++ treecontains zero
syncfsreferences. (Note in passing: the native branch at:162-200writes a.defoldtmp_*file thendmSys::Rename; the web branchskips that and writes the target directly, so web saves are not atomic either.)
fclosereaches JSFS.closethrough muslfclose.c→__stdio_close.c→ the WASI shim's
fd_close.dmloader.js— written into every HTML5 bundle byHTML5Bundler.java:476—mounts the save root as IDBFS and patches
FS.closeso every close callsModule.persistentSync()(:1201-1212), which starts an asynchronousFS.syncfs(false, cb)(:1279), coalesced via_syncInProgress/_syncNeededand retried up to
_syncMaxTries: 3. This patched close is the onlyMEM→IDB trigger in the whole system.
preSync(:1126-1152) runsFS.syncfs(true, ...)beforemain(), which iswhat makes a prior save visible at boot.
The mount:
Module.preInittakesDMSYS.GetUserPersistentDataRoot()=/data(
engine/dlib/src/dlib/js/library_sys.js:7-13),FS.mkdirs it andFS.mount(IDBFS, {}, dir)(:1185-1204);sys.get_save_fileresolves under itvia
engine/dlib/src/dlib/sys_web.cpp:59-77. IDBFS is linked bybuild_tools/waf_dynamo.py:753(-lidbfs.js).Why neither of the issue's two proposed outcomes was taken
No flush was added. The proposed
html5.run("FS.syncfs(false, function(){})")after save would start the same async operation the patched
FS.closealreadystarted. It is redundant, and it does not close the race — it is the same
fire-and-forget call.
The caveat was not deleted. The issue anticipated the warning might be false
("an unnecessary warning should be deleted rather than left to spook people").
It is not false.
FS.syncfsis asynchronous,sys.savereturns before theIndexedDB transaction commits, and there is no
beforeunload/pagehide/exitsafety net anywhere in the loader or the engine (verified absent, not assumed).
The race is real — narrower than the old text implied, and for a different
reason than it gave.
So the caveat is rewritten rather than deleted or acted on: it now names the
actual mechanism, and gives the one ordering that genuinely waits — trigger the
reload from inside the callback,
html5.run("FS.syncfs(false, function(){ location.reload() })").html5.runis confirmed the right API for that:Html5_Run(
engine/script/src/script_html5_js.cpp:77-104)evals in the engine's own JSscope, where
FSis a visible module-scope binding — notablyFSis not inEXPORTED_RUNTIME_METHODS, so a page script could not reach it, only an in-engineeval. The
html5namespace exists only on web builds(
engine/script/src/script_html5.cpp:23-25is an empty stub elsewhere), which iswhy an
is_html5()guard would be mandatory for any such call.One case the old text missed entirely
Where IndexedDB is unavailable — some private-browsing modes —
FS.mkdir/mountthrows, the loader sets
Module.persistentStorage = false(
dmloader.js:1188-1190,:1214-1217, gated at:1303), the mount is skipped,and
sys.savewrites the in-memory layer only. Zero durability, no error raised.Now documented.
Deliberately not done
The plan for this phase had a contingency to extend
dev/tools/html5-conformance.mjsto reload immediately on
SMOKE: identity, to cover the narrow window. Dropped:since no flush the SDK can issue closes that window, such an assertion would test
a race the SDK cannot win. It would land as a flaky check, not a regression guard.
Baseline
Conformance was not re-run: this diff touches no executable file.
Gaps, stated rather than guessed
Real IDB commit latency and the probability of losing the reload race are
browser- and load-dependent and not in engine source. A consumer hand-replacing
dmloader.jswith a custom loader could drop the patch. DMFS-mode custom mountsand iOS Safari IndexedDB eviction are out of source scope.
Review actioned. All five findings were correct, and the two priority-1 ones
disproved sentences I had written myself. Verdict: NEEDS ANOTHER REVIEW —
four claims changed, and since this PR's entire value is the accuracy of its
engine-source description, my own sign-off is not worth much.
Thank you for re-reading
dmloader.jsrather than trusting the paragraph. Thisis the second time this round that re-capturing the source overturned the
author's premise.
Finding 1 (retry) — fixed. The claim was mine, not the engine's.
You are right: the save-direction error path at
:1275-1299does aconsole.infoand increments_syncTries, and never retries._syncMaxTriesisa one-way circuit breaker, not a retry budget, and it is the boot direction
that retries at
:1132-1135. I had applied boot's behaviour to the save path Iwas describing. Coalescing is real and is now stated as coalescing only.
Finding 2 (the redundancy argument) — fixed, and this is the important one
"An explicit
FS.syncfswould only start the same operation again" holds on thehappy path and nowhere else. Both states you identified are real:
_syncInProgressset at:1276is never cleared,so
persistentSyncearly-returns for the lifetime of the page even afterIndexedDB recovers.
I kept the do-nothing decision, for a narrower reason now stated in the text: an
extra fire-and-forget
syncfsin a failing-IndexedDB state is not a fix — thecondition that made the first sync fail is what would make the second fail — and
it introduces the concurrency hazard your finding 3 identifies. What was wrong
was implying the loader has this covered. The README now says outright that a
save whose sync fails is silently lost, with nothing raised to Lua, which is the
warning a reader actually needs.
Finding 3 (the snippet races the loader's sync) — stated
The recommendation stays, since it is still the only ordering that waits, but it
now carries the caveat that it bypasses
_syncInProgressand can runconcurrently with the sync
sys.savejust triggered — the exact conditiondmloader.js:1273-1274says the wrapper exists to prevent. The "don't addanother
syncfs" phrasing immediately before recommending one was genuinelyconfusing; rewritten so the distinction is bare fire-and-forget versus
callback-driven.
Finding 4 (no-IndexedDB causal chain) — fixed
The mount is not skipped;
preInitnever consultspersistentStoragebeforemounting. The flag is already false from the
window.indexedDBgate at:1303,and it is
persistentSync(:1167) andpreSync(:1125) early-returning on itthat leaves
sys.savewriting only the in-memory layer. My stated outcome wasright for the wrong reason, which in a paragraph whose purpose is a verifiable
mechanism is the same as being wrong.
Finding 5 (bare basename) — fixed
dmloader.jsnow carries its full repo path,com.dynamo.cr/com.dynamo.cr.bob/src/com/dynamo/bob/bundle/resources/web/dmloader.js.Good catch that the plausible guess 404s: the one file holding the load-bearing
line numbers should be the easiest to find, or the rewrite defeats itself.
State
Still documentation-only — one file,
README.md, no SDK code.lua dev/test/runner.lua6 passed, 0 failed.
Second review round found two blocking defects, and the first is the one that matters: my bolded claim "a save whose sync fails is silently lost" is false for the single-failure case. FS.syncfs(false) takes no path - it is MOUNT-WIDE, and IDBFS diffs the whole local set against the whole remote set by mtime. So a save whose flush failed is still in MEMFS with a differing mtime, and the next flush that succeeds writes it; any FS.close anywhere in the app starts one. Loss is real only in the two terminal cases the paragraph already described separately - the page ending first, or the breaker tripping - and I had generalised those into the common case. On a docs-only change whose entire value is precision, a bolded false generalisation is the worst thing in it. The other blocking defect: the section's only copy-pasteable line, html5.run("FS.syncfs(...)"), crashes on native targets, where the html5 namespace is an empty stub (script_html5.cpp:23-25) and the call raises "attempt to index global 'html5'". It is offered for shutdown paths, which in a cross-platform game run on desktop too. The SDK already ships and documents stdb.is_html5() and uses it internally; the snippet now shows it. Six precision fixes on top: - The permanence of the wedge is _syncInProgress, NOT _syncMaxTries. Line 1297, inside the range I cited, resets _syncTries to 0, so on the counter alone the breaker would skip one flush and resume. A reader opening the citation would have concluded the doc was wrong. - "persistentSync returns early forever" is not what the cited code does: it runs through and sets _syncNeeded. The effect I described was right, the control flow was not, and I had cited :1167 for a different early return two paragraphs later. - Citations tightened: the FS.close patch is :1204-1212 not :1201-1212 (off by five in the PR body, where it covered the patch alone), and the error path is :1282-1284, not the whole of _startSyncFS. - Coalescing now says where it drops work: re-entry lives inside the syncfs callback, so a coalesced sync queued behind the flush that trips the breaker is discarded rather than deferred. That is the true version of what my overstated lede was reaching for. - The private-browsing example was wrong and was the section's one uncheckable claim: Chrome incognito, Firefox >= 115 and Safari private all DO expose window.indexedDB, so :1303 is true and they take the ordinary mount path. Private mode bites by failing open(), which is the breaker path instead. The gate is now described for what it is. - Added the three missing citations (/data, the boot sync, and FS being reachable from html5.run), and noted that html5.run returns its result as a STRING, so a mistake comes back as "ReferenceError: ..." rather than raising - a reader would otherwise have no signal the recommendation did nothing. Two additions beyond fixing errors, both from the review: - The do-nothing argument is replaced with the one that survives. "It would only start the same operation again" holds on the happy path only - review overturned that sentence once already, and standing on it again invites a third round. The real reason is that no durability signal is available to Lua at any price: sys.save returns a boolean about the MEMFS write, syncfs's only channel is a JS callback, and html5.run is a synchronous eval returning a string. Reporting durability needs an async JS->Lua bridge plus new public API, one release after freezing it, to report something whose only remedy is the consumer one-liner already documented. - The wedge now has an escape hatch instead of ending on "nothing you can do". Clearing Module._syncInProgress by hand re-arms the breaker, since :1297 has already reset the counter. Bad as SDK default behaviour - the SDK cannot tell a wedge from a legitimate in-flight sync without a timer heuristic, and would manufacture the exact overlap the guard prevents - but a legitimate consumer last resort, labelled as poking loader internals. Also states the concurrency consequence rather than just the hazard (a duplicated mount-wide pass, not a lost write, since both passes diff current state inside transactions the browser serialises), and warns against the obvious "poll _syncInProgress" fix, which never terminates in exactly the wedged state.b4a2c5a965893e588067Round two actioned — all ten findings. Both blocking ones were right, and D1 is
the best correction this section has had.
D1 (BLOCKING) — fixed. The mount-wide point is the whole thing.
FS.syncfs(false)takes no path. I had been reasoning about it as if a failedflush lost that file, when IDBFS diffs the entire local set against the entire
remote set by mtime. The failed save is still in MEMFS with a differing mtime, so
the next flush that succeeds writes it — and any
FS.closeanywhere in the appstarts one. Single failures are self-healing, and my bolded lede generalised
the two genuinely terminal cases into the common one. On a docs-only change whose
entire value is precision, that was the worst sentence in it.
D9 (BLOCKING) — fixed
The section's only copy-pasteable line crashes on native, where
html5is anempty stub. It is offered for shutdown paths, which in a cross-platform game run
on desktop too. The SDK ships
stdb.is_html5(), documents it, and uses itinternally — and my PR body even said the guard "would be mandatory" while the
README omitted it. Now shown, with the native branch.
D2, D3, D4, D5, D7, D8 — all fixed
:1297resets_syncTriesto 0, inside the range I cited. On the counter alone the breaker skips one
flush and resumes. The permanence is
_syncInProgressbeing stuck, which thenext sentence said correctly while this one misattributed it.
persistentSyncdoes not early-return in the wedged state, it runsthrough and sets
_syncNeeded. Right effect, wrong control flow, and I hadcited
:1167for a different early return two paragraphs later.:1204-1212, and the error path is:1282-1284ratherthan the whole of
_startSyncFS. Tightened.syncfs callback, so a sync queued behind the flush that trips the breaker is
discarded, not deferred. That is the accurate version of what my overstated
lede was reaching for.
html5.run-returns-a-string point too. That one is genuinely load-bearing:a mistake in that snippet comes back as
"ReferenceError: ..."instead ofraising, so a reader would have no signal the recommendation silently did
nothing.
D6 — fixed, and it was the section's one uncheckable claim
Chrome incognito, Firefox ≥115 and Safari private all expose
window.indexedDB,so
:1303is true and they take the ordinary mount path. The private-browsingexample was wrong and unverifiable from the source I cited. The gate is now
described for what it is — a non-browser or locked-down context — and private
mode is attributed to the failing-
open()path, where it actually belongs.D10 — taken
The caveat now states its consequence (a duplicated mount-wide pass, not a lost
write, since both passes diff current state inside transactions the browser
serialises) rather than ending on the hazard. I also took the poll trap, which is
the better half of the finding: "respect the guard by polling
_syncInProgress"is the obvious fix and it never terminates in exactly the wedged state. I kept the
one-liner plus the warning rather than the nine-line bounded wait — the README
should not ship a snippet longer than the thing it is guarding — but the trap is
now named so nobody writes the broken version.
Point 7 — I took your argument over mine
You are right that "it would only start the same operation again" is the weak
reason, that it holds on the happy path only, and that standing on a sentence
already overturned once invites a third round. Replaced with the one that
survives: there is no durability signal available to Lua at any price the SDK
can pay.
sys.savereturns a boolean about the MEMFS write,FS.syncfs's onlychannel is a JS callback,
html5.runis a synchronous eval returning a string —so reporting durability needs an async JS→Lua bridge plus new public API, one
release after freezing it, to report something whose only remedy is the one-liner
already documented. That is a real do-nothing rather than a shrug, which is
exactly the distinction I had failed to make.
Your three rejected alternatives all match my reading, and I took the addition:
the wedge now has an escape hatch instead of ending on "nothing you can do".
html5.run("Module._syncInProgress = false"), labelled as poking loaderinternals and gated on having seen the console warning. Bad as SDK default
behaviour for exactly your reason — the SDK cannot distinguish a wedge from a
legitimate in-flight sync without a timer heuristic, and would manufacture the
overlap the guard exists to prevent on every save — but a legitimate consumer
last resort. That was worth more than a fourth round on the prose, and I agree it
was the right trade.
State
Still documentation-only. Rebased onto current
main(cf8b430), so the diff isone file,
README.md(+95/-7).lua dev/test/runner.lua6 passed, 0 failed.Round three caught a fresh error introduced by round two's own correction, which is exactly what a third pass is for. I had written that html5.run "returns the result as a string, so a mistake here comes back as ReferenceError: ... rather than raising. Check the return value." The engine does the opposite: the EM_ASM_INT block catches the eval exception and sets a failure flag (script_html5_js.cpp:102-107), and the C++ that follows turns that flag into luaL_error (:112-117). lua_pushstring at :118 is reached only on success. So a JS exception is RE-RAISED as a Lua error and there is no return value to inspect - the advice was unactionable for the exact failure it named, and it told the reader an unguarded call fails soft when it raises, in a shutdown path. My citation stopped at :106, one line short of the flag report and four short of the luaL_error that disproves the sentence. Worse in practice: both snippets in the section evaluate to "undefined" and "false" respectively when they SUCCEED, so a reader following "check the return value" would have seen "false" and concluded the escape hatch had failed. Now states that a JS exception raises, that pcall is the wrap if you adapt it, and that the return value is not a success signal. Three more, all correct: - The syncfs callback fires on failure too. libidbfs.js wires oncomplete AND onerror/onabort to the same callback, so function(){ location.reload() } reloads even when the flush failed, losing the identity it was meant to protect - and "i.e. after the commit" read as if the callback implied one. It is an ordering guarantee, not a durability guarantee, and the snippet now takes `err` and warns rather than silently discarding it. - The escape hatch re-arms the breaker but flushes nothing by itself: _syncTries was already reset when the breaker tripped, so clearing the flag only lets the NEXT persistentSync through. It needs a following sys.save to be any use, which the text omitted entirely. - "So every sys.save does start a flush" was contradicted by the wedge paragraph eight lines below it. Qualified to the happy path. All 17 citations re-verified clean after the rewrite's renumbering, and the round-two claims - mount-wide self-healing, the coalescing drop, the wedge attribution, the escape hatch mechanism, the no-IndexedDB gate including the browser behaviour, and the do-nothing argument - all held. Not changed, and worth recording why: "native sys.save is already durable when it returns". Review noted the native path is fopen/fwrite/fclose plus an atomic rename with no fsync, so it is durable against process death but not power loss. The contrast being drawn is with an async IndexedDB commit, and demanding fsync rigor in a code comment exceeds the rigor used anywhere else here.Round three actioned. All four findings fixed.
D1 is precisely the failure mode a third round exists to catch: a fresh error
introduced by round two's own correction, in the sentence round two's response
called "genuinely load-bearing". Worth stating plainly, because it argues for the
process rather than against it.
D1 (BLOCKING) — fixed. The claim was inverted.
html5.runre-raises a JS exception as a Lua error: theEM_ASM_INTblockcatches it and sets a failure flag (
script_html5_js.cpp:102-107), and the C++after it calls
luaL_error(:112-117).lua_pushstringat:118is reachedonly on success. So "comes back as
"ReferenceError: ..."rather than raising" wasbackwards, and "check the return value" was unactionable for the exact failure it
named —
luaL_errorlongjmps, there is nothing to check.Your third consequence is the one that would actually have bitten someone: both
snippets in the section evaluate to
"undefined"and"false"on success, soa reader following my instruction would have seen
"false"from the escape hatchand concluded it had failed. The text now says a JS exception raises, that
pcallis the wrap if you adapt it, and that the return value is not a success signal.
Also noted: my citation stopped at
:106, one line short of the flag report andfour short of the
luaL_errorthat disproves the sentence. Had I read to the endof the function I would not have written it.
D2 — fixed, and the snippet changed as a result
libidbfs.jswiresoncompleteandonerror/onabortto the same callback,so
function(){ location.reload() }reloads even when the flush failed — losingthe identity the snippet exists to protect. And "i.e. after the commit" did read
as though the callback implied one. The text now says the callback fires once the
sync settles, names it an ordering guarantee rather than a durability one, and
the snippet takes
errand warns instead of silently discarding it.D3 — fixed
The escape hatch re-arms the breaker but flushes nothing by itself.
_syncTrieswas already reset when the breaker tripped, so clearing the flag only lets the
next
persistentSyncthrough. The text omitted that entirely, which made theone-liner look self-sufficient when it needs a following
sys.save. Now stated,with reconnecting given as the easy way to trigger one.
D4 — fixed
"So every
sys.savedoes start a flush" was contradicted by the wedge paragrapheight lines below it. Qualified to the happy path.
On what you re-verified
Thank you for re-checking all 17 citations after the rewrite renumbered them —
that is exactly where a silent drift would have hidden, and it is reassuring that
the mount-wide self-healing claim, the coalescing drop, the wedge attribution, the
escape-hatch mechanism, the no-IndexedDB gate including current browser behaviour,
and the do-nothing argument all held. Also useful that the mount-wide claim is if
anything conservative, since
FS.syncfsiterates all mounts rather than one.The one you considered and dropped
You were right to leave
"native sys.save is already durable when it returns"alone, and right about the underlying fact: the native path is
fopen/fwrite/fcloseplus an atomic rename with nofsync, so it is durableagainst process death but not power loss. The contrast being drawn is with an
async IndexedDB commit, and holding a code comment to fsync rigor would exceed the
rigor used anywhere else in this codebase. Recorded in the commit so the judgement
is on the record rather than an omission.
State
Documentation only, one file.
lua dev/test/runner.lua6 passed, 0 failed. Diffagainst
mainisREADME.md+108/-7.