diff --git a/src/main/java/org/ecocean/StartupWildbook.java b/src/main/java/org/ecocean/StartupWildbook.java index fab8d3306f..f07b0e40f3 100644 --- a/src/main/java/org/ecocean/StartupWildbook.java +++ b/src/main/java/org/ecocean/StartupWildbook.java @@ -886,9 +886,6 @@ public void contextDestroyed(ServletContextEvent sce) { System.out.println("* StartupWildbook destroyed called for: " + servletContextInfo(sContext)); - if (CommonConfiguration.useSpotPatternRecognition(context)) { - saveMatchGraph(sContext, context); - } // Stop the WBIA poller first so it does not race teardown of // Shepherd / IndexingManager / QueueUtil while a poll cycle is in // flight. shutdownWbiaRegisterExecutor signals shutdown by @@ -938,30 +935,18 @@ public static void createMatchGraph() { * via MatchGraphCreationThread. */ public static void loadMatchGraphOrRebuild(ServletContext sContext, String context) { - try { - String dataDir = CommonConfiguration.getDataDirectory(sContext, context).getAbsolutePath(); - String cacheFile = GridManager.getCacheFilePath(dataDir); - if (new File(cacheFile).exists() && GridManager.cacheRead(cacheFile)) { - System.out.println("INFO: matchGraph loaded from cache."); - return; - } - } catch (Exception e) { - System.out.println("WARNING: Could not load matchGraph cache, rebuilding from DB: " + - e.getMessage()); - } + // Always rebuild the match graph from the database at startup. The match + // graph is the candidate set for Modified-Groth scans; rebuilding from the + // DB on every startup guarantees deleted spot maps / encounters cannot + // reappear as match candidates (issue #1608). The former + // WEB-INF/MatchGraphCache.json startup cache was only refreshed on a + // graceful shutdown, so a crash/kill left it stale and the next startup + // resurrected deleted spots. setMatchGraphReady(false) up front keeps + // scans gated until the rebuild completes. + GridManager.setMatchGraphReady(false); createMatchGraph(); } - public static void saveMatchGraph(ServletContext sContext, String context) { - try { - String dataDir = CommonConfiguration.getDataDirectory(sContext, context).getAbsolutePath(); - String cacheFile = GridManager.getCacheFilePath(dataDir); - GridManager.cacheWrite(cacheFile); - } catch (Exception e) { - System.out.println("WARNING: Could not save matchGraph cache: " + e.getMessage()); - } - } - public static boolean skipInit(ServletContextEvent sce, String extra) { ServletContext sc = sce.getServletContext(); diff --git a/src/main/java/org/ecocean/grid/GridManager.java b/src/main/java/org/ecocean/grid/GridManager.java index abbce2193a..97f78373ad 100644 --- a/src/main/java/org/ecocean/grid/GridManager.java +++ b/src/main/java/org/ecocean/grid/GridManager.java @@ -2,13 +2,10 @@ // import org.apache.commons.math.stat.descriptive.SummaryStatistics; // import org.ecocean.CommonConfiguration; -import org.ecocean.Util; import org.ecocean.shepherd.core.Shepherd; // import org.ecocean.servlet.ServletUtilities; -import java.io.IOException; -import java.util.Iterator; import java.util.concurrent.ConcurrentHashMap; import javax.servlet.http.HttpServletRequest; @@ -16,7 +13,6 @@ import java.util.ArrayList; import java.util.Enumeration; -import org.json.JSONObject; // import org.apache.commons.math.stat.descriptive.SummaryStatistics; @@ -666,77 +662,9 @@ public int getNumUnderway() { } public void resetMatchGraphWithInitialCapacity(int initialCapacity) { - matchGraph = null; + // Single assignment: never expose a null matchGraph to concurrent mutators + // (servlet add/remove) during a rebuild -- the old null-then-new sequence + // had an NPE window. (#1608) matchGraph = new ConcurrentHashMap(initialCapacity); } - - private static final String CACHE_FILEPATH = "WEB-INF/MatchGraphCache.json"; - - public static String getCacheFilePath(String dataDir) { - return dataDir + "/" + CACHE_FILEPATH; - } - - /** - * Serialize the matchGraph to a JSONObject for disk caching. - */ - public static JSONObject cacheToJSONObject() { - JSONObject root = new JSONObject(); - root.put("timestamp", System.currentTimeMillis()); - JSONObject entries = new JSONObject(); - for (ConcurrentHashMap.Entry entry : matchGraph.entrySet()) { - entries.put(entry.getKey(), entry.getValue().toJSONObject()); - } - root.put("matchGraph", entries); - root.put("count", matchGraph.size()); - return root; - } - - /** - * Write the matchGraph cache to disk as JSON. - */ - public static void cacheWrite(String filepath) throws IOException { - long t = System.currentTimeMillis(); - System.out.println("INFO: GridManager.cacheWrite() writing to " + filepath); - Util.writeToFile(cacheToJSONObject().toString(), filepath); - System.out.println("INFO: GridManager.cacheWrite() complete with " + matchGraph.size() + - " entries in " + (System.currentTimeMillis() - t) + "ms"); - } - - /** - * Read the matchGraph cache from disk JSON. - * Returns true if the cache was loaded successfully, false otherwise. - */ - public static boolean cacheRead(String filepath) throws IOException { - long t = System.currentTimeMillis(); - matchGraphReady = false; - String content = Util.readFromFile(filepath); - JSONObject root = Util.stringToJSONObject(content); - if (root == null) { - System.out.println("ERROR: GridManager.cacheRead() could not parse " + filepath); - return false; - } - System.out.println("INFO: GridManager.cacheRead() from " + filepath + - " timestamp=" + root.optLong("timestamp")); - - JSONObject entries = root.optJSONObject("matchGraph"); - if (entries == null || entries.length() < 1) { - System.out.println("ERROR: GridManager.cacheRead() empty matchGraph in " + filepath); - return false; - } - - matchGraph = new ConcurrentHashMap(entries.length()); - Iterator keys = entries.keys(); - while (keys.hasNext()) { - String key = keys.next(); - JSONObject elJson = entries.optJSONObject(key); - if (elJson == null) continue; - EncounterLite el = EncounterLite.fromJSONObject(elJson); - matchGraph.put(key, el); - } - resetPatternCounts(); - matchGraphReady = true; - System.out.println("INFO: GridManager.cacheRead() complete with " + matchGraph.size() + - " entries in " + (System.currentTimeMillis() - t) + "ms"); - return true; - } } diff --git a/src/main/java/org/ecocean/grid/MatchGraphCreationThread.java b/src/main/java/org/ecocean/grid/MatchGraphCreationThread.java index 610121ff33..cf0447d78f 100644 --- a/src/main/java/org/ecocean/grid/MatchGraphCreationThread.java +++ b/src/main/java/org/ecocean/grid/MatchGraphCreationThread.java @@ -74,6 +74,14 @@ public void createThem() { for (int i = 0; i < numEncs; i++) { Encounter enc = myShepherd.getEncounter(encNumbers.get(i)); + // getEncounter() returns null if the encounter was deleted after the + // id snapshot above (a concurrent EncounterDelete during the rebuild) + // or otherwise could not be fetched. Skip it rather than NPE-aborting + // the whole rebuild. A genuine exception while building the + // EncounterLite below is NOT swallowed: it propagates to the outer + // catch, which aborts the rebuild WITHOUT flipping matchGraphReady to + // true, so we never publish a silently-incomplete graph. (#1608) + if (enc == null) continue; if (((enc.getRightSpots() != null) && (enc.getRightSpots().size() > 0)) || ((enc.getSpots() != null) && (enc.getSpots().size() > 0))) { EncounterLite el = new EncounterLite(enc); diff --git a/src/main/java/org/ecocean/servlet/EncounterDelete.java b/src/main/java/org/ecocean/servlet/EncounterDelete.java index 02309c2123..2f362ccb71 100644 --- a/src/main/java/org/ecocean/servlet/EncounterDelete.java +++ b/src/main/java/org/ecocean/servlet/EncounterDelete.java @@ -56,6 +56,22 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) PrintWriter out = response.getWriter(); boolean locked = false; + // Refuse deletion while the match graph is rebuilding from the DB (only + // for ~15 min after a restart on spot-matching installs). A concurrent + // deletion during the rebuild could be undone by the rebuild's stale read + // and resurrect the encounter as a match candidate. (#1608) + if (CommonConfiguration.useSpotPatternRecognition(context) && + !GridManager.isMatchGraphReady()) { + myShepherd.closeDBTransaction(); // PM opened in ctor; no tx begun yet + out.println(ServletUtilities.getHeader(request)); + out.println( + "Please wait: the match graph is rebuilding. Please try deleting this encounter again in a few minutes."); + out.println(ServletUtilities.getFooter(context)); + response.setStatus(HttpServletResponse.SC_SERVICE_UNAVAILABLE); + out.close(); + return; + } + // setup data dir String rootWebappPath = getServletContext().getRealPath("/"); File webappsDir = new File(rootWebappPath).getParentFile(); @@ -171,11 +187,13 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) enc2trash.setSkipAutoIndexing(false); myShepherd.throwAwayEncounter(enc2trash); - // remove from grid too - GridManager gm = GridManagerFactory.getGridManager(); - gm.removeMatchGraphEntry(request.getParameter("number")); - - myShepherd.commitDBTransaction(); + // Only drop the match-graph entry once the deletion is confirmed + // committed, so a swallowed commit failure can't remove a + // still-present encounter from the candidate set. (#1608) + if (myShepherd.commitDBTransactionWithStatus()) { + GridManager gm = GridManagerFactory.getGridManager(); + gm.removeMatchGraphEntry(request.getParameter("number")); + } // log it log.info("Click to restore deleted encounter: Please wait: the match graph is rebuilding. Please try removing spot data again in a few minutes."); + out.println(ServletUtilities.getFooter(context)); + response.setStatus(HttpServletResponse.SC_SERVICE_UNAVAILABLE); + out.close(); + return; + } // ConcurrentHashMap chm= gm.getMatchGraph(); /* if(request.getParameter("number")!=null){ @@ -97,9 +112,27 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) "-side spot data.

"); // despotMe.setNumLeftSpots(0); } - gm.addMatchGraphEntry(request.getParameter("number"), - new EncounterLite(despotMe)); - myShepherd.commitDBTransaction(); + // Build the lite snapshot while the encounter is still managed, + // then update the in-memory match graph AFTER the DB commit so + // (a) a failed commit can't leave the graph inconsistent with the + // DB and (b) the startup rebuild's DB read can't overwrite this + // change with pre-commit (still-spotted) state. (#1608) + boolean stillHasSpots = + ((despotMe.getSpots() != null) && (despotMe.getSpots().size() > 0)) || + ((despotMe.getRightSpots() != null) && + (despotMe.getRightSpots().size() > 0)); + EncounterLite updatedLite = stillHasSpots ? new EncounterLite(despotMe) : null; + // Only touch the match graph once the DB change is confirmed + // committed, so a swallowed commit failure can't leave the graph + // inconsistent with the DB. (#1608) + if (myShepherd.commitDBTransactionWithStatus()) { + if (stillHasSpots) { + gm.addMatchGraphEntry(request.getParameter("number"), updatedLite); + } else { + // fully de-spotted: drop it from the candidate set entirely + gm.removeMatchGraphEntry(request.getParameter("number")); + } + } } else { locked = true; myShepherd.rollbackDBTransaction(); diff --git a/src/main/java/org/ecocean/servlet/GrothMatchServlet.java b/src/main/java/org/ecocean/servlet/GrothMatchServlet.java index 818a950639..2cac4f7667 100644 --- a/src/main/java/org/ecocean/servlet/GrothMatchServlet.java +++ b/src/main/java/org/ecocean/servlet/GrothMatchServlet.java @@ -126,6 +126,11 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) if (entry.getKey().equals(encNumber)) continue; EncounterLite el = entry.getValue(); + // Skip candidates with no spots on the side being scanned. An encounter + // whose spot map was deleted must never surface as a match candidate. (#1608) + ArrayList candidateSpots = rightScan ? el.getRightSpots() : el.getSpots(); + if ((candidateSpots == null) || candidateSpots.isEmpty()) continue; + MatchObject mo = el.getPointsForBestMatch( queryArray, epsilon, R, Sizelim, maxTriangleRotation, C, true, rightScan); diff --git a/src/main/java/org/ecocean/servlet/SubmitSpotsAndImage.java b/src/main/java/org/ecocean/servlet/SubmitSpotsAndImage.java index 45c4277e98..19fb2fd665 100644 --- a/src/main/java/org/ecocean/servlet/SubmitSpotsAndImage.java +++ b/src/main/java/org/ecocean/servlet/SubmitSpotsAndImage.java @@ -165,12 +165,20 @@ public void doPost(HttpServletRequest request, HttpServletResponse response) enc.setSpots(spots); enc.setLeftReferenceSpots(refSpots); } - // reset the entry in the GridManager graph + // Build the lite snapshot while the encounter is still managed, then + // update the in-memory match graph AFTER the DB commit so a failed commit + // can't leave the graph inconsistent with the DB and the startup rebuild + // can't overwrite it with pre-commit state. (#1608) GridManager gm = GridManagerFactory.getGridManager(); - gm.addMatchGraphEntry(encId, new EncounterLite(enc)); - - myShepherd.commitDBTransaction(); + EncounterLite updatedLite = new EncounterLite(enc); + + // Only add to the match graph once the spots are confirmed committed, so + // a swallowed commit failure can't leave a phantom candidate. (#1608) + boolean committed = myShepherd.commitDBTransactionWithStatus(); myShepherd.closeDBTransaction(); + if (committed) { + gm.addMatchGraphEntry(encId, updatedLite); + } JSONObject rtn = new JSONObject("{\"success\": true}"); rtn.put("spotAssetId", crMa.getId()); diff --git a/src/main/java/org/ecocean/shepherd/core/Shepherd.java b/src/main/java/org/ecocean/shepherd/core/Shepherd.java index 32a4c9582b..fe8a1d0cb4 100644 --- a/src/main/java/org/ecocean/shepherd/core/Shepherd.java +++ b/src/main/java/org/ecocean/shepherd/core/Shepherd.java @@ -3358,6 +3358,30 @@ public void commitDBTransaction() { } } + /** + * Commit and report whether the commit actually succeeded. Unlike + * {@link #commitDBTransaction()} (which swallows commit failures and returns + * void), this returns false if the transaction was inactive or the commit + * threw. Callers can use this to avoid mutating in-memory state after a commit + * that did not durably persist -- e.g. only update the GridManager match graph + * once the encounter change is confirmed committed. (#1608) + */ + public boolean commitDBTransactionWithStatus() { + try { + if ((pm != null) && pm.currentTransaction().isActive()) { + pm.currentTransaction().commit(); + ShepherdState.setShepherdState(action + "_" + shepherdID, "commit"); + return true; + } + System.out.println("commitDBTransactionWithStatus: transaction was not active."); + return false; + } catch (Exception e) { + System.out.println("commitDBTransactionWithStatus: commit failed: " + e); + e.printStackTrace(); + return false; + } + } + /** * Since we call these together all over Wildbook */