R-351: the restore compares where the backup says the data lived; second press cannot start a second run
gates / gates (push) Successful in 10s
gates / gates (push) Successful in 10s
Part 3 (not droppable) and the engine half of Part 2. No version bump yet - one bump and
one bake at the end of the session.
PART 3a - a second press really did start a second run. Established with a test BEFORE any
change: both offboxReconstituteHandler and offboxPlaceHandler answered "...elindult" and
overwrote the first restore's op/stack. Cause: every restore handler gated on
backupMgr.IsRunning() - the CONCURRENCY flag, which the restore goroutine acquires AFTER the
handler returns (offbox_reconstitute.go:180, offbox_restore.go:393). Seven sites. The wizard
had read the correct flag since v0.154.0 and said so in a comment; the handlers never moved.
New Server.restoreOpBlocked() reads BOTH flags - the display flag covers the whole off-box
restore, the concurrency flag is the only one the nightly backup holds - and the refusal now
names the running app and a route.
PART 3b - the page DOES refresh; the defect was the RESULT. backups_shared.html gated the
terminal result on a page-local sawRunning flag, so a restore that finished before the page
was opened, or inside one 3s poll, was shown to nobody. The 2026-08-21 OpenGist restore took
8.666s and no screen ever said it completed - the answer existed only in docker logs.
RestoreOpStatus.LastRecent now carries the server's verdict. The 10-minute window moved to
internal/backup as RestoreResultWindow and internal/web's constant is an alias: one
expression, two surfaces. Also removed the wizard's self-contradiction, which said the state
refreshes automatically AND that you must refresh the page.
PART 2 (engine) - every recovery unit manifest has carried drive and namespace_root since
schema 1, and NO non-test code read either back. The reconstitution opened the manifest and
took only the coherence stamp, then resolved its destination from the live app. A restore
into a different destination succeeded silently under a green message. New
backup/offbox_placement.go: CheckPlacement (pure, total), PlacementMismatchMessage,
recordedPlacementFromScratch. Compared before the safety dump and before the first byte.
A mismatch is NAMED and refused; ackPlacementChange lets the customer proceed deliberately -
a separate field from confirm=1, because one click must not carry two decisions. An UNKNOWN
recording is never a mismatch: refusing on an absence would strand every pre-field unit.
The not-installed refusal (R-253) now names the drive the backup recorded.
RED-PROOFS, each mutation asserted applied and reverted to 0:
B both guards removed (count asserted 2) -> the restore WAS seen starting with no drive
attached: no error, full 3.00s run, wrote into /tmp/mutant-destination
C Mismatch forced false -> the silent divergent restore returned
E Known() forced true -> the fabricated empty prefill appeared
D Mismatch forced true -> 8 ordinary reconstitute tests broke, proving reachability both ways
Note on D: the existing fixtures write a schema-1 manifest with NO drive, so they are
scenario-E shaped. The matching case is covered in the scenario table, not by them.
Gates 11/11 OK. Suite 28 packages ok. Hungarian verified as hex, no BOM, no mojibake sentinels.
NOT in this commit, still open: Part 2's scenario-A prefill UI, Part 1's deploy-page
visibility line, Part 1's specification document, Part 4's measurement.
This commit is contained in:
@@ -1401,8 +1401,8 @@ func (s *Server) backupRestoreHandler(w http.ResponseWriter, r *http.Request) {
|
||||
// Part B: restore is a long SYNCHRONOUS op (F4 — through cloudflared's hard 100s cap the customer
|
||||
// got an error page while it silently succeeded). Fast-path refuse a concurrent op, then run it in
|
||||
// a BACKGROUND goroutine (survives the request; the poll banner shows progress → result).
|
||||
if s.backupMgr.IsRunning() {
|
||||
http.Redirect(w, r, "/backups/restore?flash_error="+url.QueryEscape("Egy mentési/visszaállítási művelet már fut."), http.StatusFound)
|
||||
if msg, blocked := s.restoreOpBlocked(); blocked {
|
||||
http.Redirect(w, r, "/backups/restore?flash_error="+url.QueryEscape(msg), http.StatusFound)
|
||||
return
|
||||
}
|
||||
s.logger.Printf("[WARN] [web] Restore requested (async): stack=%s, snapshot=%s from %s", stackName, snapshotID, r.RemoteAddr)
|
||||
@@ -1460,8 +1460,8 @@ func (s *Server) backupTier2RestoreHandler(w http.ResponseWriter, r *http.Reques
|
||||
return
|
||||
}
|
||||
// Part B (same async shape as backupRestoreHandler): fast-path refuse, then background goroutine.
|
||||
if s.backupMgr.IsRunning() {
|
||||
http.Redirect(w, r, "/backups/apps?flash_error="+url.QueryEscape("Egy mentési/visszaállítási művelet már fut."), http.StatusFound)
|
||||
if msg, blocked := s.restoreOpBlocked(); blocked {
|
||||
http.Redirect(w, r, "/backups/apps?flash_error="+url.QueryEscape(msg), http.StatusFound)
|
||||
return
|
||||
}
|
||||
|
||||
|
||||
@@ -352,10 +352,10 @@ func (s *Server) offboxRestoreHandler(w http.ResponseWriter, r *http.Request) {
|
||||
}
|
||||
// Fast-path refuse a concurrent op, then run async on a BACKGROUND context (a proxy read-timeout on
|
||||
// r.Context() would CANCEL the SFTP restore mid-flight — the F4 lesson).
|
||||
if s.backupMgr.IsRunning() {
|
||||
if msg, blocked := s.restoreOpBlocked(); blocked {
|
||||
// Same silence class as the size gate: a refusal that starts nothing must still be findable.
|
||||
s.logger.Printf("[WARN] [web] off-box restore refused for %s (mode=%s): another backup/restore op is already running", app, mode)
|
||||
offboxRedirectTo(w, r, restoreWizardPath(app), "Egy mentési/visszaállítási művelet már fut.", true)
|
||||
offboxRedirectTo(w, r, restoreWizardPath(app), msg, true)
|
||||
return
|
||||
}
|
||||
full := mode == "full"
|
||||
@@ -433,15 +433,19 @@ func (s *Server) offboxReconstituteHandler(w http.ResponseWriter, r *http.Reques
|
||||
offboxRedirectTo(w, r, restoreWizardPath(app), "A teljes visszaállítás megerősítés nélkül nem hajtható végre.", true)
|
||||
return
|
||||
}
|
||||
if s.backupMgr.IsRunning() {
|
||||
offboxRedirectTo(w, r, restoreWizardPath(app), "Egy mentési/visszaállítási művelet már fut.", true)
|
||||
if msg, blocked := s.restoreOpBlocked(); blocked {
|
||||
offboxRedirectTo(w, r, restoreWizardPath(app), msg, true)
|
||||
return
|
||||
}
|
||||
// R-351: a SEPARATE field from `confirm`. The restore's own confirm answers "overwrite my live
|
||||
// data"; this one answers "yes, into a different place than the backup recorded". One checkbox
|
||||
// carrying both would be the two-decisions-one-button shape R-48 removed from this surface.
|
||||
ackPlacement := r.FormValue("ack_placement") == "1"
|
||||
s.backupMgr.BeginRestoreOp("offbox-reconstitute", app)
|
||||
go func() {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), 60*time.Minute)
|
||||
defer cancel()
|
||||
res, err := s.backupMgr.ReconstituteFromOffsite(ctx, app)
|
||||
res, err := s.backupMgr.ReconstituteFromOffsite(ctx, app, ackPlacement)
|
||||
if err != nil {
|
||||
s.logger.Printf("[ERROR] [web] off-box reconstitute %s (async): %v", app, err)
|
||||
s.backupMgr.EndRestoreOp(false, "A teljes visszaállítás sikertelen: "+err.Error())
|
||||
@@ -521,8 +525,8 @@ func (s *Server) offboxPlaceHandler(w http.ResponseWriter, r *http.Request) {
|
||||
offboxRedirectTo(w, r, "/backups/restore", "Hiányzó alkalmazás.", true)
|
||||
return
|
||||
}
|
||||
if s.backupMgr.IsRunning() {
|
||||
offboxRedirectTo(w, r, restoreWizardPath(app), "Egy mentési/visszaállítási művelet már fut.", true)
|
||||
if msg, blocked := s.restoreOpBlocked(); blocked {
|
||||
offboxRedirectTo(w, r, restoreWizardPath(app), msg, true)
|
||||
return
|
||||
}
|
||||
s.backupMgr.BeginRestoreOp("offbox-place", app)
|
||||
@@ -554,8 +558,8 @@ func (s *Server) sharesRestoreHandler(w http.ResponseWriter, r *http.Request) {
|
||||
offboxRedirectTo(w, r, "/backups/restore", "A távoli mentési cél nincs beállítva.", true)
|
||||
return
|
||||
}
|
||||
if s.backupMgr.IsRunning() {
|
||||
offboxRedirectTo(w, r, "/backups/restore", "Egy mentési/visszaállítási művelet már fut.", true)
|
||||
if msg, blocked := s.restoreOpBlocked(); blocked {
|
||||
offboxRedirectTo(w, r, "/backups/restore", msg, true)
|
||||
return
|
||||
}
|
||||
s.backupMgr.BeginRestoreOp("shares-restore", backup.SharesDisplayName)
|
||||
@@ -580,8 +584,8 @@ func (s *Server) sharesPlaceHandler(w http.ResponseWriter, r *http.Request) {
|
||||
offboxRedirectTo(w, r, "/backups/restore", "A távoli mentési cél nincs beállítva.", true)
|
||||
return
|
||||
}
|
||||
if s.backupMgr.IsRunning() {
|
||||
offboxRedirectTo(w, r, "/backups/restore", "Egy mentési/visszaállítási művelet már fut.", true)
|
||||
if msg, blocked := s.restoreOpBlocked(); blocked {
|
||||
offboxRedirectTo(w, r, "/backups/restore", msg, true)
|
||||
return
|
||||
}
|
||||
s.backupMgr.BeginRestoreOp("shares-place", backup.SharesDisplayName)
|
||||
|
||||
@@ -0,0 +1,116 @@
|
||||
package web
|
||||
|
||||
import (
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-controller/internal/settings"
|
||||
)
|
||||
|
||||
// R-351 — THE SECOND PRESS. Part 3 asked whether a second press really starts a second run or is
|
||||
// refused somewhere deeper. It is NOT refused: it starts a second run, and the customer is told so.
|
||||
//
|
||||
// WHY THE EXISTING GUARD DOES NOT CATCH IT. Every restore handler gates on `backupMgr.IsRunning()`
|
||||
// (offbox_handlers.go:355/436/524/557/583, handlers.go:1404/1463). That reads `m.running`, the
|
||||
// CONCURRENCY single-flight, which is acquired INSIDE the restore function on the background
|
||||
// goroutine (offbox_reconstitute.go:180, offbox_restore.go:393) — not by the handler. So between the
|
||||
// handler's check and the goroutine's acquire there is a window in which `IsRunning()` is false while
|
||||
// a restore is unmistakably in flight. `restoreOpInFlight` and the whole wizard already read the
|
||||
// other flag (`RestoreStatus().Running`, set synchronously by BeginRestoreOp) for exactly this
|
||||
// reason — see the long note on restoreOpInFlight. The handlers were never moved over.
|
||||
//
|
||||
// THE FIXTURE IS THE LIVE STATE, NOT AN INVENTED ONE: display flag set, concurrency flag NOT held.
|
||||
// That is precisely what the box looks like for the entire duration of an off-box restore.
|
||||
//
|
||||
// This test pins the CONSEQUENCE (does a second run start?), not the mechanism (which flag is read),
|
||||
// per CLAUDE.md's preference. Mutating the new guard back to `IsRunning()` must make it fail.
|
||||
func TestRestoreHandlers_SecondPressDoesNotStartASecondRun(t *testing.T) {
|
||||
const firstApp = "alpha"
|
||||
const secondApp = "beta"
|
||||
|
||||
s, sett, m := newOffboxWebServer(t)
|
||||
if err := sett.SetOffboxTarget(&settings.OffboxTarget{
|
||||
Enabled: true, Host: "nas.local", Port: 22, User: "felhom", RepoPath: "/srv/repo",
|
||||
Schedule: "daily", EscrowState: "escrowed",
|
||||
}); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if err := m.WriteOffboxSecrets("PRIVATE-KEY-MATERIAL", "nas.local ssh-ed25519 AAAAhostkey"); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if !m.OffboxConfigured() {
|
||||
t.Fatal("fixture: the target must be configured, or the handler exits earlier and proves nothing")
|
||||
}
|
||||
|
||||
// A restore is in flight, exactly as the live box has it.
|
||||
m.BeginRestoreOp("offbox-restore", firstApp)
|
||||
|
||||
// The fixture must reproduce the GAP, or this test is vacuous: the display flag says running,
|
||||
// the concurrency flag — the one every handler reads — says it is not.
|
||||
if !m.RestoreStatus().Running {
|
||||
t.Fatal("fixture: the display flag must report a running op")
|
||||
}
|
||||
if m.IsRunning() {
|
||||
t.Fatal("fixture: the concurrency flag must NOT be held — that gap IS the defect under test")
|
||||
}
|
||||
|
||||
post := func(path string, form url.Values) *httptest.ResponseRecorder {
|
||||
r := httptest.NewRequest("POST", path, strings.NewReader(form.Encode()))
|
||||
r.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
w := httptest.NewRecorder()
|
||||
switch path {
|
||||
case "/backup/offbox/reconstitute":
|
||||
s.offboxReconstituteHandler(w, r)
|
||||
case "/backup/offbox/place":
|
||||
s.offboxPlaceHandler(w, r)
|
||||
default:
|
||||
t.Fatalf("unrouted path %q", path)
|
||||
}
|
||||
return w
|
||||
}
|
||||
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
path string
|
||||
form url.Values
|
||||
}{
|
||||
{"reconstitute", "/backup/offbox/reconstitute", url.Values{"app": {secondApp}, "confirm": {"1"}}},
|
||||
{"place", "/backup/offbox/place", url.Values{"app": {secondApp}}},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
w := post(tc.path, tc.form)
|
||||
if w.Code != 302 {
|
||||
t.Fatalf("the handler redirects; got %d", w.Code)
|
||||
}
|
||||
loc := w.Header().Get("Location")
|
||||
|
||||
// CONSEQUENCE 1 — the customer must not be told a second restore started.
|
||||
// "elind" is the ASCII stem shared by every „elindult" flash; matching it avoids putting
|
||||
// accented bytes through a comparison (strict rule 7).
|
||||
if strings.Contains(loc, "elind") {
|
||||
t.Errorf("a second press while a restore runs must NOT report a started restore. Location: %q", loc)
|
||||
}
|
||||
|
||||
// CONSEQUENCE 2 — the in-flight op must still be the FIRST one. If the handler ran,
|
||||
// BeginRestoreOp overwrote the op name and stack, so the first restore's identity is
|
||||
// gone from the status the banner reads.
|
||||
st := m.RestoreStatus()
|
||||
if st.Op != "offbox-restore" || st.Stack != firstApp {
|
||||
t.Errorf("the first restore's identity was overwritten by the second press: op=%q stack=%q (want offbox-restore/%s)",
|
||||
st.Op, st.Stack, firstApp)
|
||||
}
|
||||
|
||||
// CONSEQUENCE 3 — a refusal must name a route the person can act on, not just a reason.
|
||||
// The wizard path is the route; it is where the live status is shown.
|
||||
if !strings.Contains(loc, "/backups/restore") {
|
||||
t.Errorf("the refusal must route somewhere actionable; got %q", loc)
|
||||
}
|
||||
|
||||
// Restore the fixture for the next subtest — a handler that (today) ran will have
|
||||
// clobbered it.
|
||||
m.BeginRestoreOp("offbox-restore", firstApp)
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -113,7 +113,12 @@ const (
|
||||
// Without a bound the last result would light that phase forever — landing on the page a week later
|
||||
// would claim you had just finished a restore. Same reasoning as escrowCeremonyGraceWindow; shorter,
|
||||
// because this answers "what just happened", not "are we still waiting".
|
||||
const restoreResultWindow = 10 * time.Minute
|
||||
//
|
||||
// R-351: this is now an ALIAS, not a second value. The list page's banner needs the same bound, and
|
||||
// the payload carries the verdict (RestoreOpStatus.LastRecent), so the window is defined once in
|
||||
// internal/backup beside the status it bounds. Keeping a separate literal here is how the two
|
||||
// surfaces would drift.
|
||||
const restoreResultWindow = backup.RestoreResultWindow
|
||||
|
||||
// hasRecentRestoreResult reports whether THIS app has a just-finished restore to show. Pure (the
|
||||
// clock is a parameter) so the boundary and the wrong-app case are table-testable.
|
||||
@@ -182,6 +187,43 @@ func restoreOpInFlight(st backup.RestoreOpStatus) bool {
|
||||
return st.Running
|
||||
}
|
||||
|
||||
// restoreOpBlocked reports whether a NEW restore must be refused right now, and returns the
|
||||
// Hungarian refusal to show. It reads BOTH flags, deliberately:
|
||||
//
|
||||
// - `RestoreStatus().Running` — the DISPLAY flag, set synchronously by `BeginRestoreOp` in the
|
||||
// handler. It is the only one that is true for the WHOLE duration of an off-box restore, which
|
||||
// is what makes it the right flag to refuse on.
|
||||
// - `IsRunning()` — the CONCURRENCY flag. The nightly backup run holds this one and never calls
|
||||
// `BeginRestoreOp`, so dropping it would open a hole the old guard did close. Kept, not replaced.
|
||||
//
|
||||
// R-351, the measured defect: every restore handler read ONLY `IsRunning()`, which the restore
|
||||
// goroutine acquires AFTER the handler has already returned (offbox_reconstitute.go:180,
|
||||
// offbox_restore.go:393). A second press inside that window started a second run and was told
|
||||
// „…elindult". Pinned by TestRestoreHandlers_SecondPressDoesNotStartASecondRun, which asserts the
|
||||
// CONSEQUENCE — that the first restore's identity survives the second press — rather than which
|
||||
// flag was read.
|
||||
//
|
||||
// The refusal names a reason AND a route: the page it redirects to is the wizard, which carries the
|
||||
// live status banner, so „ezen az oldalon" is a true instruction and not a gesture.
|
||||
func (s *Server) restoreOpBlocked() (string, bool) {
|
||||
if s.backupMgr == nil {
|
||||
return "", false
|
||||
}
|
||||
if st := s.backupMgr.RestoreStatus(); restoreOpInFlight(st) {
|
||||
subject := "Egy visszaállítási művelet"
|
||||
if st.Stack != "" {
|
||||
subject = "Egy visszaállítási művelet (" + st.Stack + ")"
|
||||
}
|
||||
return subject + " már fut, ezért most nem indítható újabb. Az állapotát ezen az oldalon " +
|
||||
"követheted; amint befejeződik, újra indíthatsz visszaállítást.", true
|
||||
}
|
||||
if s.backupMgr.IsRunning() {
|
||||
return "Egy mentési művelet már fut, ezért most nem indítható visszaállítás. Az állapotát " +
|
||||
"ezen az oldalon követheted; amint befejeződik, újra indíthatsz visszaállítást.", true
|
||||
}
|
||||
return "", false
|
||||
}
|
||||
|
||||
// backupsRestoreWizardHandler renders GET /backups/restore/app?name=<app> — the single entry the
|
||||
// list page now offers per app.
|
||||
//
|
||||
|
||||
@@ -45,7 +45,11 @@
|
||||
<div class="settings-card">
|
||||
<h3>Végrehajtás</h3>
|
||||
<p>Jelenleg egy mentési vagy visszaállítási művelet fut{{with .RunningStack}} ({{.}}){{end}}. Amíg ez tart, új visszaállítás nem indítható.</p>
|
||||
<p class="form-hint">Az állapot fent automatikusan frissül. A művelet befejezése után frissítsd az oldalt.</p>
|
||||
<!-- R-351: this used to say the state refreshes automatically AND that you must refresh the page
|
||||
when it finishes. Both cannot be true, and the second half was the one people believed. The
|
||||
banner now carries the terminal result too (RestoreOpStatus.LastRecent), so the sentence can
|
||||
describe what the page actually does. -->
|
||||
<p class="form-hint">Az állapot fent automatikusan frissül, és a művelet eredménye is ott jelenik meg, amint elkészült.</p>
|
||||
<div class="form-actions">
|
||||
<a href="/backups/restore/app?name={{.App}}" class="btn btn-sm btn-outline">Állapot frissítése</a>
|
||||
</div>
|
||||
|
||||
@@ -40,7 +40,11 @@
|
||||
banner.textContent = opLabel(st.op) + ' folyamatban' + (st.stack ? ': ' + st.stack : '') + '…';
|
||||
return;
|
||||
}
|
||||
if (st.last && sawRunning) {
|
||||
// R-351: `sawRunning` alone showed a terminal result ONLY to a page that watched the op
|
||||
// happen. A restore that finished before this page was opened — or inside one poll interval
|
||||
// — was shown to nobody, which is how a completed restore became unknowable from any screen.
|
||||
// `last_recent` is the server's verdict, using the one window in internal/backup.
|
||||
if (st.last && (sawRunning || st.last_recent)) {
|
||||
banner.style.display = 'block';
|
||||
if (st.last.ok) {
|
||||
banner.className = 'flash flash-success';
|
||||
|
||||
Reference in New Issue
Block a user