Replay a dirty journal before writing to an instance image#64
Merged
Conversation
BlueStacks instances are almost never shut down cleanly. The player is
terminated rather than asked to close, since its own clean exit needs a human to
click through a confirmation dialog, so the guest never unmounts and /data comes
to rest with an unreplayed journal. Booting replays it, which is why this stays
invisible in normal use.
Offline it broke things twice over. e2fsck -fn cannot replay a journal, so it
reported such an image as damaged:
Warning: skipping journal recovery because doing a read-only filesystem check
Inode 3410023 was part of the orphaned inode list. IGNORED.
Block bitmap differences: ...
Those errors are phantoms, the journal's own pending metadata, and every caller
turned them into "e2fsck reported errors" and refused to work: an operation
declined because of how the instance was last closed, not because anything was
wrong. Restart an instance, then install Magisk, and it would fail for no
reason. Worse, writing with debugfs into a filesystem whose journal still holds
pending metadata risks the replay at next boot overwriting what was just
written.
So attach now replays first, and every offline writer inherits it because they
all funnel through _Attached.
The repair must run on the partition device, not the ?offset= one used
everywhere else. After replaying, e2fsck reopens the filesystem to restart its
check, and the reopen drops the ?offset= qualifier, lands on the raw disk, and
dies with "Bad magic number in super-block" before committing. The replay then
never persists: run it twice and it announces "recovering journal" both times
with the image byte-for-byte unchanged. Pointed at /dev/sdX1 the same preen
completes and sticks. The partition is identified by matching e2fsck's capacity
totals against the offset device, so a repair can never land on another
filesystem, and the verdict always comes from re-checking rather than from
preen's exit code, which is 12 even on success.
Verified live: an image left dirty by a real kill reports not-clean, gets the
replay, verifies clean, and a second attach finds nothing to do.
Contributor
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Review of the first pass found the repair could make things worse than the bug it fixed. Corrections: Never raise. An unreplayed journal is the normal resting state of these images, so failing hard would blanket-block every offline write on a host where the partition node is not exposed, and it would also block the uninstall and restore paths, which tolerate a dirty filesystem on purpose (remove_su_symlink runs _fsck_ok and discards the result precisely so a removal always completes). Refusing to let someone undo a change because their last shutdown was untidy is worse than the original bug. When the replay cannot happen we are simply no worse off than before, and callers keep their own post-write verification. Identify the partition by superblock UUID rather than by matching e2fsck's size totals. Two partitions of the same size produced the same fingerprint, which is exactly the mix-up the guard exists to prevent. Reading the UUID with debugfs is also a superblock read rather than a full check pass, which removes four to six walks of a multi-GB image per attach: the old code re-ran the identical -fn twice and then checked every candidate partition. Reattach between the preen and the writes. The preen goes through the partition node while debugfs writes through the disk node, and on Windows those are separate device objects with separate caches, so a late write-back of stale sectors could land on top of what was just written. Log the preen's exit code and output. Discarding them meant a repair that never ran was reported to the user as filesystem damage, with nothing in the log to say otherwise. Report the repair note through the caller's progress reporter, so a long replay is visible instead of looking frozen on "Attaching...", and honour _detach's result on the failure path so a stuck raw disk is not hidden behind an unrelated error. Tests now cover _Attached itself, which is where the behaviour change actually lives: the repair flag, the detach-on-failure path, the reattach, and the note. 266 green.
The cache-refresh step after a journal replay detached and re-attached without checking whether the detach succeeded. _detach returns False after four retries when diskpart cannot release the disk, and re-attaching something still attached would only compound that. Keep the working device and log why the cache was not dropped instead. This is the same omission that was already fixed on the error path; the refresh path was missed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
BlueStacks instances are almost never shut down cleanly. The player is terminated rather than asked to close (its own clean exit needs a human to click through a confirmation dialog), so the guest never unmounts and
/datacomes to rest with an unreplayed journal. Booting replays it, which is why this is invisible in normal use.Offline it broke two things.
1. Operations were refused for no reason.
e2fsck -fncannot replay a journal, so it reports such an image as damaged:Those are phantoms: the journal's own pending metadata, which evaporates once replayed. Every caller turned them into
e2fsck reported errors. Restart an instance, then install Magisk, and it fails because of how the instance was last closed.2. Writes could be silently clobbered. Writing with
debugfsinto a filesystem whose journal still holds pending metadata risks the replay at next boot overwriting what was just written. This is the likeliest explanation for module binaries previously turning up zero-length.The fix
_Attached.__enter__now replays the journal before handing out the device, so every offline writer inherits it (ext4_symlink,magisk_system,telemetry_blockall funnel through it). A clean image is left completely untouched.The repair has to run on the partition device, not the
?offset=one. After replaying, e2fsck reopens the filesystem to restart its check; the reopen drops the?offset=qualifier, lands on the raw disk, and dies before committing:The replay never persists. Run it twice and it announces "recovering journal" both times, with the image byte-for-byte unchanged. Pointed at the partition there is nothing to lose on reopen:
Two safeguards:
Verification
Live, on an image left dirty by a real kill:
259 tests green, ruff clean. The new tests cover routing to the partition device, the identity guard against repairing a different filesystem, exit-code independence across 0/1/2/4/8/12, and refusing to write when the image is still dirty afterwards.